Refactor SSLClientCertificateSelector for reuse with platformKeys API.

chrome/browser/ui/views/certificate_selector.cc

Index: chrome/browser/ui/views/certificate_selector.cc
diff --git a/chrome/browser/ui/views/ssl_client_certificate_selector.cc b/chrome/browser/ui/views/certificate_selector.cc
similarity index 31%
copy from chrome/browser/ui/views/ssl_client_certificate_selector.cc
copy to chrome/browser/ui/views/certificate_selector.cc
index de31ce8f02a3c46810ae1a838aa29b848d1bfbf0..8bf01d245629c777036d08f5377e2e13cd946667 100644
--- a/chrome/browser/ui/views/ssl_client_certificate_selector.cc
+++ b/chrome/browser/ui/views/certificate_selector.cc
@@ -1,19 +1,16 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ui/views/ssl_client_certificate_selector.h"
+#include "chrome/browser/ui/views/certificate_selector.h"
 
-#include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/certificate_viewer.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/constrained_window/constrained_window_views.h"
-#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 #include "net/cert/x509_certificate.h"
-#include "net/ssl/ssl_cert_request_info.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/models/table_model.h"
 #include "ui/base/models/table_model_observer.h"
@@ -25,19 +22,13 @@
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/dialog_client_view.h"
 
-#if defined(USE_NSS)
-#include "chrome/browser/ui/crypto_module_password_dialog_nss.h"
-#endif
-
-///////////////////////////////////////////////////////////////////////////////
-// CertificateSelectorTableModel:
+namespace chrome {
 
-class CertificateSelectorTableModel : public ui::TableModel {
+class CertificateSelector::CertificateTableModel : public ui::TableModel {
  public:
-  explicit CertificateSelectorTableModel(
-      net::SSLCertRequestInfo* cert_request_info);
+  explicit CertificateTableModel(const net::CertificateList& certificates);
 
-  // ui::TableModel implementation:
+  // ui::TableModel:
   int RowCount() override;
   base::string16 GetText(int index, int column_id) override;
   void SetObserver(ui::TableModelObserver* observer) override;
@@ -45,14 +36,13 @@ class CertificateSelectorTableModel : public ui::TableModel {
  private:
   std::vector<base::string16> items_;
 
-  DISALLOW_COPY_AND_ASSIGN(CertificateSelectorTableModel);
+  DISALLOW_COPY_AND_ASSIGN(CertificateTableModel);
 };
 
-CertificateSelectorTableModel::CertificateSelectorTableModel(
-    net::SSLCertRequestInfo* cert_request_info) {
-  for (size_t i = 0; i < cert_request_info->client_certs.size(); ++i) {
-    net::X509Certificate* cert = cert_request_info->client_certs[i].get();
-    base::string16 text = l10n_util::GetStringFUTF16(
+CertificateSelector::CertificateTableModel::CertificateTableModel(
+    const net::CertificateList& certificates) {
+  for (const scoped_refptr<net::X509Certificate>& cert : certificates) {

[bartfab (slow), 2015/02/19 17:53:06]

Why not |auto|?

[pneubeck (no reviews), 2015/02/19 19:43:51]

I find it helpful to document the element type, in particular if the container
type is already typedef'ed away.

+    const base::string16 text = l10n_util::GetStringFUTF16(

[msw, 2015/02/19 19:53:53]

nit: inline this in the push_back call.

[pneubeck (no reviews), 2015/02/19 20:43:49]

Done.

         IDS_CERT_SELECTOR_TABLE_CERT_FORMAT,
         base::UTF8ToUTF16(cert->subject().GetDisplayName()),
         base::UTF8ToUTF16(cert->issuer().GetDisplayName()));
@@ -60,58 +50,48 @@ CertificateSelectorTableModel::CertificateSelectorTableModel(
   }
 }
 
-int CertificateSelectorTableModel::RowCount() {
+int CertificateSelector::CertificateTableModel::RowCount() {
   return items_.size();
 }
 
-base::string16 CertificateSelectorTableModel::GetText(int index,
-                                                      int column_id) {
+base::string16 CertificateSelector::CertificateTableModel::GetText(
+    int index,
+    int column_id) {
   DCHECK_EQ(column_id, 0);
   DCHECK_GE(index, 0);
-  DCHECK_LT(index, static_cast<int>(items_.size()));
+  DCHECK_LT(static_cast<size_t>(index), items_.size());
 
   return items_[index];
 }
 
-void CertificateSelectorTableModel::SetObserver(
+void CertificateSelector::CertificateTableModel::SetObserver(
     ui::TableModelObserver* observer) {
 }
 
-///////////////////////////////////////////////////////////////////////////////
-// SSLClientCertificateSelector:
-
-SSLClientCertificateSelector::SSLClientCertificateSelector(
-    content::WebContents* web_contents,
-    const scoped_refptr<net::SSLCertRequestInfo>& cert_request_info,
-    const chrome::SelectCertificateCallback& callback)
-    : SSLClientAuthObserver(web_contents->GetBrowserContext(),
-                            cert_request_info, callback),
-      model_(new CertificateSelectorTableModel(cert_request_info.get())),
-      web_contents_(web_contents),
-      table_(NULL),
-      view_cert_button_(NULL) {
-  DVLOG(1) << __FUNCTION__;
+CertificateSelector::CertificateSelector(
+    const net::CertificateList& certificates,
+    content::WebContents* web_contents)
+    : certificates_(certificates),
+      model_(new CertificateTableModel(certificates)),
+      web_contents_(web_contents) {
+  CHECK(web_contents_);
 }
 
-SSLClientCertificateSelector::~SSLClientCertificateSelector() {
-  table_->SetModel(NULL);
+CertificateSelector::~CertificateSelector() {
+  table_->SetModel(nullptr);
 }
 
-void SSLClientCertificateSelector::Init() {
-  views::GridLayout* layout = views::GridLayout::CreatePanel(this);
+void CertificateSelector::Init(const base::string16& text) {
+  views::GridLayout* const layout = views::GridLayout::CreatePanel(this);

[bartfab (slow), 2015/02/19 17:53:07]

Nit: Use a |scoped_ptr| while you are holding ownership.

[pneubeck (no reviews), 2015/02/19 19:43:51]

impractical in this case.
The ownership is passed on to SetLayoutManager in the following line.
The alternative is super bulky:

scoped_ptr<views::GridLayout>
owned_layout(views::GridLayout::CreatePanel(this));
views::GridLayout* const layout = owned_layout.get();
SetLayoutManager(owned_layout.release());
...

[bartfab (slow), 2015/02/20 12:19:47]

That super bulky code is what I tend to do :). But Views is pretty lenient on
raw pointers and ownership anyway, so SGTM to just stay consistent with Views
style.

   SetLayoutManager(layout);
 
   const int column_set_id = 0;

[bartfab (slow), 2015/02/19 17:53:06]

Nit 1: Use are using inconsistent naming for constant,s |column_set_id| vs.
|kTableViewWidth|.
Nit 2: Define constants at the top of the file.

[pneubeck (no reviews), 2015/02/19 19:43:51]

Done.
Not sure what you intend to improve by that. This go against the typical attempt
to define stuff where you use it.

-  views::ColumnSet* column_set = layout->AddColumnSet(column_set_id);
-  column_set->AddColumn(
-      views::GridLayout::FILL, views::GridLayout::FILL,
-      1, views::GridLayout::USE_PREF, 0, 0);
+  views::ColumnSet* const column_set = layout->AddColumnSet(column_set_id);
+  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,

[bartfab (slow), 2015/02/19 17:53:06]

If you have a single column only, why not use a BoxLayout?

[pneubeck (no reviews), 2015/02/19 19:43:51]

Agreed. I can try that in a follow up. This CL tries not to change the existing
SSLClientCertificateSelector dialog.

+                        views::GridLayout::USE_PREF, 0, 0);
 
   layout->StartRow(0, column_set_id);
-  base::string16 text = l10n_util::GetStringFUTF16(
-      IDS_CLIENT_CERT_DIALOG_TEXT,
-      base::ASCIIToUTF16(cert_request_info()->host_and_port.ToString()));
-  views::Label* label = new views::Label(text);
+  views::Label* const label = new views::Label(text);

[bartfab (slow), 2015/02/19 17:53:06]

Nit: Use a |scoped_ptr| while you are holding ownership.

[pneubeck (no reviews), 2015/02/19 19:43:51]

Done.

   label->SetMultiLine(true);
   label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   label->SetAllowCharacterBreak(true);
@@ -123,16 +103,14 @@ void SSLClientCertificateSelector::Init() {
   static const int kTableViewWidth = 400;
   static const int kTableViewHeight = 100;
 
-  CreateCertTable();
+  table_ = CreateCertTable();
   layout->StartRow(1, column_set_id);
   layout->AddView(table_->CreateParentIfNecessary(), 1, 1,
-                  views::GridLayout::FILL,
-                  views::GridLayout::FILL, kTableViewWidth, kTableViewHeight);
+                  views::GridLayout::FILL, views::GridLayout::FILL,
+                  kTableViewWidth, kTableViewHeight);
 
   layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
 
-  StartObserving();
-
   constrained_window::ShowWebModalDialogViews(this, web_contents_);
 
   // Select the first row automatically.  This must be done after the dialog has
@@ -140,146 +118,75 @@ void SSLClientCertificateSelector::Init() {
   table_->Select(0);
 }
 
-net::X509Certificate* SSLClientCertificateSelector::GetSelectedCert() const {
-  int selected = table_->FirstSelectedRow();
-  if (selected >= 0 &&
-      selected < static_cast<int>(cert_request_info()->client_certs.size()))
-    return cert_request_info()->client_certs[selected].get();
-  return NULL;
-}
-
-///////////////////////////////////////////////////////////////////////////////
-// SSLClientAuthObserver implementation:
-
-void SSLClientCertificateSelector::OnCertSelectedByNotification() {
-  DVLOG(1) << __FUNCTION__;
-  GetWidget()->Close();
+net::X509Certificate* CertificateSelector::GetSelectedCert() const {
+  const int selected = table_->FirstSelectedRow();
+  if (selected >= 0 && static_cast<size_t>(selected) < certificates_.size())

[msw, 2015/02/19 19:53:53]

We should be able to [D]CHECK_LT(selected, certificates_.size()), right?
I guess maybe it would fail if called before Init()...

[pneubeck (no reviews), 2015/02/19 20:43:49]

A call before Init() will crash because of table_ == nullptr.
I added a comment to the declaration.
Done.

+    return certificates_[selected].get();
+  return nullptr;
 }
 
-///////////////////////////////////////////////////////////////////////////////
-// DialogDelegateView implementation:
-
-bool SSLClientCertificateSelector::CanResize() const {
+bool CertificateSelector::CanResize() const {
   return true;
 }
 
-base::string16 SSLClientCertificateSelector::GetWindowTitle() const {
+base::string16 CertificateSelector::GetWindowTitle() const {
   return l10n_util::GetStringUTF16(IDS_CLIENT_CERT_DIALOG_TITLE);
 }
 
-void SSLClientCertificateSelector::DeleteDelegate() {
+void CertificateSelector::DeleteDelegate() {
   DVLOG(1) << __FUNCTION__;

[bartfab (slow), 2015/02/19 17:53:06]

Do we expect crashes around this line?

[pneubeck (no reviews), 2015/02/19 19:43:51]

I don't know, kept this from the SSLClientCertificateSelector.

[msw, 2015/02/19 19:53:53]

If not, this override isn't needed. DialogDelegateView's impl would suffice.

[pneubeck (no reviews), 2015/02/19 20:43:49]

good point. removed.

   delete this;
 }
 
-bool SSLClientCertificateSelector::IsDialogButtonEnabled(
-    ui::DialogButton button) const {
+bool CertificateSelector::IsDialogButtonEnabled(ui::DialogButton button) const {
   if (button == ui::DIALOG_BUTTON_OK)

[msw, 2015/02/19 19:53:53]

nit: return button != ui::DIALOG_BUTTON_OK || GetSelectedCert() != nullptr;

[pneubeck (no reviews), 2015/02/19 20:43:49]

Done.

-    return !!GetSelectedCert();
+    return GetSelectedCert() != nullptr;
   return true;
 }
 
-bool SSLClientCertificateSelector::Cancel() {
-  DVLOG(1) << __FUNCTION__;
-  StopObserving();
-  CertificateSelected(NULL);
-  return true;
-}
-
-bool SSLClientCertificateSelector::Accept() {
-  DVLOG(1) << __FUNCTION__;
-  scoped_refptr<net::X509Certificate> cert = GetSelectedCert();
-  if (cert.get()) {
-    // Remove the observer before we try unlocking, otherwise we might act on a
-    // notification while waiting for the unlock dialog, causing us to delete
-    // ourself before the Unlocked callback gets called.
-    StopObserving();
-#if defined(USE_NSS)
-    chrome::UnlockCertSlotIfNecessary(
-        cert.get(),
-        chrome::kCryptoModulePasswordClientAuth,
-        cert_request_info()->host_and_port,
-        GetWidget()->GetNativeView(),
-        base::Bind(&SSLClientCertificateSelector::Unlocked,
-                   base::Unretained(this),
-                   cert));
-#else
-    Unlocked(cert.get());
-#endif
-    return false;  // Unlocked() will close the dialog.
-  }
-
-  return false;
-}
-
-views::View* SSLClientCertificateSelector::GetInitiallyFocusedView() {
+views::View* CertificateSelector::GetInitiallyFocusedView() {
+  DCHECK(table_);
   return table_;
 }
 
-views::View* SSLClientCertificateSelector::CreateExtraView() {
+views::View* CertificateSelector::CreateExtraView() {
   DCHECK(!view_cert_button_);
-  view_cert_button_ = new views::LabelButton(this,
-      l10n_util::GetStringUTF16(IDS_PAGEINFO_CERT_INFO_BUTTON));
+  view_cert_button_ = new views::LabelButton(

[bartfab (slow), 2015/02/19 17:53:06]

Nit: Use a |scoped_ptr| while you are holding ownership.

[pneubeck (no reviews), 2015/02/19 19:43:51]

Done.

+      this, l10n_util::GetStringUTF16(IDS_PAGEINFO_CERT_INFO_BUTTON));
   view_cert_button_->SetStyle(views::Button::STYLE_BUTTON);
   return view_cert_button_;
 }
 
-ui::ModalType SSLClientCertificateSelector::GetModalType() const {
+ui::ModalType CertificateSelector::GetModalType() const {
   return ui::MODAL_TYPE_CHILD;
 }
 
-///////////////////////////////////////////////////////////////////////////////
-// views::ButtonListener implementation:
-
-void SSLClientCertificateSelector::ButtonPressed(
-    views::Button* sender, const ui::Event& event) {
+void CertificateSelector::ButtonPressed(views::Button* sender,
+                                        const ui::Event& event) {
   if (sender == view_cert_button_) {
-    net::X509Certificate* cert = GetSelectedCert();
+    net::X509Certificate* const cert = GetSelectedCert();
     if (cert)
       ShowCertificateViewer(web_contents_,
-                            web_contents_->GetTopLevelNativeWindow(),
-                            cert);
+                            web_contents_->GetTopLevelNativeWindow(), cert);
   }
 }
 
-///////////////////////////////////////////////////////////////////////////////
-// views::TableViewObserver implementation:
-void SSLClientCertificateSelector::OnSelectionChanged() {
-  GetDialogClientView()->ok_button()->SetEnabled(!!GetSelectedCert());
+void CertificateSelector::OnSelectionChanged() {
+  GetDialogClientView()->ok_button()->SetEnabled(GetSelectedCert() != nullptr);
 }
 
-void SSLClientCertificateSelector::OnDoubleClick() {
+void CertificateSelector::OnDoubleClick() {
   if (Accept())
     GetWidget()->Close();
 }
 
-///////////////////////////////////////////////////////////////////////////////
-// SSLClientCertificateSelector private methods:
-
-void SSLClientCertificateSelector::CreateCertTable() {
+views::TableView* CertificateSelector::CreateCertTable() {
   std::vector<ui::TableColumn> columns;
   columns.push_back(ui::TableColumn());
-  table_ = new views::TableView(model_.get(), columns, views::TEXT_ONLY,
-                                true /* single_selection */);
-  table_->SetObserver(this);
-}
-
-void SSLClientCertificateSelector::Unlocked(net::X509Certificate* cert) {
-  DVLOG(1) << __FUNCTION__;
-  CertificateSelected(cert);
-  GetWidget()->Close();
-}
-
-namespace chrome {
-
-void ShowSSLClientCertificateSelector(
-    content::WebContents* contents,
-    net::SSLCertRequestInfo* cert_request_info,
-    const chrome::SelectCertificateCallback& callback) {
-  DVLOG(1) << __FUNCTION__ << " " << contents;
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  (new SSLClientCertificateSelector(
-       contents, cert_request_info, callback))->Init();
+  views::TableView* const table = new views::TableView(

[bartfab (slow), 2015/02/19 17:53:06]

Nit: Use a |scoped_ptr| while you are holding ownership.

[pneubeck (no reviews), 2015/02/19 19:43:51]

Done.

[msw, 2015/02/19 19:53:53]

Why not assign directly to table_, as this code used to do, or inline in Init?

[pneubeck (no reviews), 2015/02/19 20:43:49]

The old one was weird with respect to ownership and it was not
documented/obvious that it modifies some state.

inlining seems fine.
Done.

+      model_.get(), columns, views::TEXT_ONLY, true /* single_selection */);
+  table->SetObserver(this);
+  return table;
 }
 
 }  // namespace chrome