platformKeys: Hook up the certificate selection dialog to selectClientCertificates.

platformKeys: Hook up the certificate selection dialog to selectClientCertificates.

With this change the platformKeys API is functional.
The only restriction it is effectively disabled if the user is policy managed.
This restriction will be removed once the according policy is in place.

Depends on
https://codereview.chromium.org/932553002/

BUG=450167
Committed: https://crrev.com/fcaf87dba8e3ae20df648d169eb379d38697a552
Cr-Commit-Position: refs/heads/master@{#317335}

[pneubeck (no reviews), 2015-02-17 15:34:55]

Patchset #1 (id:1) has been deleted

[pneubeck (no reviews), 2015-02-17 15:45:07]

Patchset #1 (id:20001) has been deleted

[pneubeck (no reviews), 2015-02-19 17:14:55]

Patchset #2 (id:60001) has been deleted

[pneubeck (no reviews), 2015-02-19 17:15:00]

Patchset #1 (id:40001) has been deleted

[pneubeck (no reviews), 2015-02-19 17:19:43]

pneubeck@chromium.org changed reviewers:
+ bartfab@chromium.org

[pneubeck (no reviews), 2015-02-19 17:19:43]

ptal

[bartfab (slow), 2015-02-19 18:55:43]

lgtm with nits.

https://codereview.chromium.org/927293002/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/927293002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9981: +      <message
name="IDS_PLATFORM_KEYS_SELECT_CERT_DIALOG_TEXT" desc="The text in the dialog
that prompts for a certificate when an extension requested certificates through
the platformKeys API.">
Nit: s/prompts for/prompts the user to select/
Nit: s/requested/requests/

https://codereview.chromium.org/927293002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9982: +        Extension '<ph
name="EXTENSION_NAME">$1<ex>Better OpenVPN App</ex>' has requested a
certificate. Selecting a certificate will grant the extension the ability to use
this identity with servers now and in the future. You should only select a
certificate if you trust the extension.
The </ph> belongs at the end of the placeholder, not at the end of the message.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys_service.h (right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:67:
content::WebContents* web_contents,
Nit: Document that |web_contents| will not be a nullptr and provides the context
for which a certificate is needed.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:175:
content::WebContents* web_contents);
Nit: Document that |web_contents| must not be a nullptr.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc
(right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:43: const
extensions::Extension* extension =
Nit 1: s/const extensions::Extension*/const extensions::Extension* const/
Nit 2: #include "extensions/common/extension.h"

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:45:
extension_id, extensions::ExtensionRegistry::ENABLED);
What if the extension is not found? At least a DCHECK() would make sense.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:56: const
scoped_refptr<net::X509Certificate>& selected_cert) {
Nit 1: #include "base/memory/ref_counted.h"
Nit 2: #include "net/cert/x509_certificate.h"

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:63: };
Nit: DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:101:
policy::ProfilePolicyConnector* connector =
Nit: const.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:103: //
Only allow the user to grant certificate permissions to extensions, if user
Nit: s/, if/if the/

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:106: //
TODO(pneubeck): Remove this once the respective policy is implemented.
Nit: Reference a crbug number.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:107:
service->SetSelectDelegate(make_scoped_ptr(
Nit: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
File chrome/browser/ui/platform_keys_certificate_selector_chromeos.h (right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
chrome/browser/ui/platform_keys_certificate_selector_chromeos.h:19: typedef
std::vector<scoped_refptr<X509Certificate>> CertificateList;
Nit: #include "base/memory/ref_counted.h"

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
chrome/browser/ui/platform_keys_certificate_selector_chromeos.h:24: // Opens a
constrained client certificate selection dialog under |parent|,
Nit: There is no |parent| argument. Did you mean |web_contents|?

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
chrome/browser/ui/platform_keys_certificate_selector_chromeos.h:25: // offering
|certficicates| to the user and explaining that the selection will
Nit: s/certficicates/certificates/

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
chrome/browser/ui/platform_keys_certificate_selector_chromeos.h:29: // the user
cancels the dialog, |callback| will be called with null.
Nit: s/null/a nullptr argument/

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc
(right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:29: const
base::string16 text =
Nit: #include "base/strings/string16.h"

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:42: if
(!cert.get())
Nit: Are scoped-refptrs not testable, i.e. |if(!cert)|?

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h
(right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:27: using
CertificateSelectedCallback = base::Callback<void(
Nit: You keep redefinining this type. Would it not have been possible to define
it once and for all in a header?

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:30:
PlatformKeysCertificateSelector(const net::CertificateList& certificates,
Nit: Forward-declare |net::CertificateList|.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:44: const
CertificateSelectedCallback callback_;
Nit: You need base/callback.h instead of base/callback_forward.h for this.

[pneubeck (no reviews), 2015-02-19 18:56:38]

pneubeck@chromium.org changed reviewers:
+ msw@chromium.org

[pneubeck (no reviews), 2015-02-19 18:56:39]

@Mike, this is the second CL that I mentioned. Bartosz is currently looking at
it, again I'll need an owner review.

[pneubeck (no reviews), 2015-02-19 18:57:06]

On 2015/02/19 18:56:39, pneubeck wrote:
> @Mike, this is the second CL that I mentioned. Bartosz is currently looking at
> it, again I'll need an owner review.

ah. he's done already.

[msw, 2015-02-19 20:21:54]

https://codereview.chromium.org/927293002/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/927293002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9980: +      <!-- Strings for the certificate
selection dialog triggered by platformKeys -->
You're really supposed to have new strings in 2 weeks before branch... Good luck
getting translations.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc
(right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:38: if
(!enabled_) {
Consider defining a separate no-op SelectDelegate subclass instead of using an
|enabled_| flag to cripple this class.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:57:
callback.Run(selected_cert);
Shouldn't this DCHECK(enabled_) or ensure that |selected_cert| is null/invalid
if |enabled_| is false? (or maybe ignore |selected_cert| altogether in that case
and run the callback with nullptr?)

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
File chrome/browser/ui/platform_keys_certificate_selector_chromeos.h (right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
chrome/browser/ui/platform_keys_certificate_selector_chromeos.h:34: const
base::Callback<void(const scoped_refptr<net::X509Certificate>&)>&
nit: maybe make a typedef for this callback to use here and elsewhere?

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h
(right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:25: class
PlatformKeysCertificateSelector : public chrome::CertificateSelector {
Add a class comment, consider adding a warning that users should use
browser/ui/platform_keys_certificate_selector_chromeos.h instead of using this
class/header, similar to the comment in ssl_client_certificate_selector.h

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:36: void
Init();
nit: try to avoid overloading this name with the base class.

[pneubeck (no reviews), 2015-02-19 21:48:47]

New patchsets have been uploaded after l-g-t-m from bartfab@chromium.org

[pneubeck (no reviews), 2015-02-19 21:51:42]

https://codereview.chromium.org/927293002/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/927293002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9980: +      <!-- Strings for the certificate
selection dialog triggered by platformKeys -->
On 2015/02/19 20:21:54, msw wrote:
> You're really supposed to have new strings in 2 weeks before branch... Good
luck
> getting translations.

Acknowledged.

https://codereview.chromium.org/927293002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9981: +      <message
name="IDS_PLATFORM_KEYS_SELECT_CERT_DIALOG_TEXT" desc="The text in the dialog
that prompts for a certificate when an extension requested certificates through
the platformKeys API.">
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: s/prompts for/prompts the user to select/
> Nit: s/requested/requests/

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9982: +        Extension '<ph
name="EXTENSION_NAME">$1<ex>Better OpenVPN App</ex>' has requested a
certificate. Selecting a certificate will grant the extension the ability to use
this identity with servers now and in the future. You should only select a
certificate if you trust the extension.
On 2015/02/19 18:55:42, bartfab wrote:
> The </ph> belongs at the end of the placeholder, not at the end of the
message.

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys_service.h (right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:67:
content::WebContents* web_contents,
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: Document that |web_contents| will not be a nullptr and provides the
context
> for which a certificate is needed.

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:175:
content::WebContents* web_contents);
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: Document that |web_contents| must not be a nullptr.

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc
(right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:38: if
(!enabled_) {
On 2015/02/19 20:21:54, msw wrote:
> Consider defining a separate no-op SelectDelegate subclass instead of using an
> |enabled_| flag to cripple this class.

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:43: const
extensions::Extension* extension =
On 2015/02/19 18:55:42, bartfab wrote:
> Nit 1: s/const extensions::Extension*/const extensions::Extension* const/
> Nit 2: #include "extensions/common/extension.h"

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:45:
extension_id, extensions::ExtensionRegistry::ENABLED);
On 2015/02/19 18:55:42, bartfab wrote:
> What if the extension is not found? At least a DCHECK() would make sense.

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:56: const
scoped_refptr<net::X509Certificate>& selected_cert) {
On 2015/02/19 18:55:42, bartfab wrote:
> Nit 1: #include "base/memory/ref_counted.h"
> Nit 2: #include "net/cert/x509_certificate.h"

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:57:
callback.Run(selected_cert);
On 2015/02/19 20:21:54, msw wrote:
> Shouldn't this DCHECK(enabled_) or ensure that |selected_cert| is null/invalid
> if |enabled_| is false? (or maybe ignore |selected_cert| altogether in that
case
> and run the callback with nullptr?)

not required anymore.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:63: };
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:101:
policy::ProfilePolicyConnector* connector =
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:103: //
Only allow the user to grant certificate permissions to extensions, if user
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: s/, if/if the/

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:106: //
TODO(pneubeck): Remove this once the respective policy is implemented.
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: Reference a crbug number.

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:107:
service->SetSelectDelegate(make_scoped_ptr(
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
File chrome/browser/ui/platform_keys_certificate_selector_chromeos.h (right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
chrome/browser/ui/platform_keys_certificate_selector_chromeos.h:19: typedef
std::vector<scoped_refptr<X509Certificate>> CertificateList;
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: #include "base/memory/ref_counted.h"

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
chrome/browser/ui/platform_keys_certificate_selector_chromeos.h:24: // Opens a
constrained client certificate selection dialog under |parent|,
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: There is no |parent| argument. Did you mean |web_contents|?

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
chrome/browser/ui/platform_keys_certificate_selector_chromeos.h:25: // offering
|certficicates| to the user and explaining that the selection will
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: s/certficicates/certificates/

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
chrome/browser/ui/platform_keys_certificate_selector_chromeos.h:29: // the user
cancels the dialog, |callback| will be called with null.
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: s/null/a nullptr argument/

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/platfo...
chrome/browser/ui/platform_keys_certificate_selector_chromeos.h:34: const
base::Callback<void(const scoped_refptr<net::X509Certificate>&)>&
On 2015/02/19 20:21:54, msw wrote:
> nit: maybe make a typedef for this callback to use here and elsewhere?

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc
(right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:29: const
base::string16 text =
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: #include "base/strings/string16.h"

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:42: if
(!cert.get())
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: Are scoped-refptrs not testable, i.e. |if(!cert)|?

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h
(right):

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:25: class
PlatformKeysCertificateSelector : public chrome::CertificateSelector {
On 2015/02/19 20:21:54, msw wrote:
> Add a class comment, consider adding a warning that users should use
> browser/ui/platform_keys_certificate_selector_chromeos.h instead of using this
> class/header, similar to the comment in ssl_client_certificate_selector.h

considering the DEPS rules that should be impossible.
to be safe and for documenting how to actually use this class, I added the
comment.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:27: using
CertificateSelectedCallback = base::Callback<void(
On 2015/02/19 18:55:42, bartfab wrote:
> Nit: You keep redefinining this type. Would it not have been possible to
define
> it once and for all in a header?

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:30:
PlatformKeysCertificateSelector(const net::CertificateList& certificates,
On 2015/02/19 18:55:43, bartfab wrote:
> Nit: Forward-declare |net::CertificateList|.

Following another comment of Mike, including x509_certificate.h

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:36: void
Init();
On 2015/02/19 20:21:54, msw wrote:
> nit: try to avoid overloading this name with the base class.

Done.

https://codereview.chromium.org/927293002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:44: const
CertificateSelectedCallback callback_;
On 2015/02/19 18:55:43, bartfab wrote:
> Nit: You need base/callback.h instead of base/callback_forward.h for this.

Done.

[msw, 2015-02-19 21:59:28]

lgtm

[pneubeck (no reviews), 2015-02-19 21:59:34]

Thanks!

[bartfab (slow), 2015-02-20 12:49:19]

Still LGTM.

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc
(right):

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:38:
callback.Run(nullptr);
Nit: #include "base/callback.h" as you are dereferncing the type.

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc
(right):

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:9:
#include "chrome/browser/ui/platform_keys_certificate_selector_chromeos.h"
Nit: Already included by header.

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h
(right):

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:10:
#include "base/callback.h"
Nit: Not used.

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:12:
#include "base/memory/ref_counted.h"
Nit: This is used in the implementation file only now.

[pneubeck (no reviews), 2015-02-20 14:09:58]

New patchsets have been uploaded after l-g-t-m from
msw@chromium.org,bartfab@chromium.org

[pneubeck (no reviews), 2015-02-20 14:25:21]

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc
(right):

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc:38:
callback.Run(nullptr);
On 2015/02/20 12:49:19, bartfab wrote:
> Nit: #include "base/callback.h" as you are dereferncing the type.

Done.

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc
(right):

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:9:
#include "chrome/browser/ui/platform_keys_certificate_selector_chromeos.h"
On 2015/02/20 12:49:19, bartfab wrote:
> Nit: Already included by header.

Done.

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h
(right):

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:10:
#include "base/callback.h"
On 2015/02/20 12:49:19, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/927293002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.h:12:
#include "base/memory/ref_counted.h"
On 2015/02/20 12:49:19, bartfab wrote:
> Nit: This is used in the implementation file only now.

Done.

[pneubeck (no reviews), 2015-02-20 15:00:31]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2015-02-20 15:01:29]

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

[pneubeck (no reviews), 2015-02-20 15:36:43]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2015-02-20 15:38:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927293002/140001

[commit-bot: I haz the power, 2015-02-20 16:31:40]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2015-02-20 16:32:14]

Patchset 4 (id:??) landed as
https://crrev.com/fcaf87dba8e3ae20df648d169eb379d38697a552
Cr-Commit-Position: refs/heads/master@{#317335}