Refactor SSLClientCertificateSelector for reuse with platformKeys API.

chrome/browser/ui/views/ssl_client_certificate_selector.cc

Index: chrome/browser/ui/views/ssl_client_certificate_selector.cc
diff --git a/chrome/browser/ui/views/ssl_client_certificate_selector.cc b/chrome/browser/ui/views/ssl_client_certificate_selector.cc
index de31ce8f02a3c46810ae1a838aa29b848d1bfbf0..bdbf7549d791033d2e14970d7a1fb5a8abbbf6b4 100644
--- a/chrome/browser/ui/views/ssl_client_certificate_selector.cc
+++ b/chrome/browser/ui/views/ssl_client_certificate_selector.cc
@@ -4,19 +4,14 @@
 
 #include "chrome/browser/ui/views/ssl_client_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"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/table/table_view.h"
@@ -29,241 +24,71 @@
 #include "chrome/browser/ui/crypto_module_password_dialog_nss.h"
 #endif
 
-///////////////////////////////////////////////////////////////////////////////
-// CertificateSelectorTableModel:
-
-class CertificateSelectorTableModel : public ui::TableModel {
- public:
-  explicit CertificateSelectorTableModel(
-      net::SSLCertRequestInfo* cert_request_info);
-
-  // ui::TableModel implementation:
-  int RowCount() override;
-  base::string16 GetText(int index, int column_id) override;
-  void SetObserver(ui::TableModelObserver* observer) override;
-
- private:
-  std::vector<base::string16> items_;
-
-  DISALLOW_COPY_AND_ASSIGN(CertificateSelectorTableModel);
-};
-
-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(
-        IDS_CERT_SELECTOR_TABLE_CERT_FORMAT,
-        base::UTF8ToUTF16(cert->subject().GetDisplayName()),
-        base::UTF8ToUTF16(cert->issuer().GetDisplayName()));
-    items_.push_back(text);
-  }
-}
-
-int CertificateSelectorTableModel::RowCount() {
-  return items_.size();
-}
-
-base::string16 CertificateSelectorTableModel::GetText(int index,
-                                                      int column_id) {
-  DCHECK_EQ(column_id, 0);
-  DCHECK_GE(index, 0);
-  DCHECK_LT(index, static_cast<int>(items_.size()));
-
-  return items_[index];
-}
-
-void CertificateSelectorTableModel::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) {
+    : CertificateSelector(
+          web_contents,
+          cert_request_info->client_certs,
+          base::Bind(&SSLClientCertificateSelector::OnCertificateSelected,
+                     base::Unretained(this))),
+      SSLClientAuthObserver(web_contents->GetBrowserContext(),
+                            cert_request_info,
+                            callback) {
   DVLOG(1) << __FUNCTION__;
 }
 
 SSLClientCertificateSelector::~SSLClientCertificateSelector() {
-  table_->SetModel(NULL);
 }
 
 void SSLClientCertificateSelector::Init() {
-  views::GridLayout* layout = views::GridLayout::CreatePanel(this);
-  SetLayoutManager(layout);
-
-  const int column_set_id = 0;
-  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);
-
-  layout->StartRow(0, column_set_id);
+  StartObserving();
   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);
-  label->SetMultiLine(true);
-  label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  label->SetAllowCharacterBreak(true);
-  layout->AddView(label);
-
-  layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
-
-  // The dimensions of the certificate selector table view, in pixels.
-  static const int kTableViewWidth = 400;
-  static const int kTableViewHeight = 100;
-
-  CreateCertTable();
-  layout->StartRow(1, column_set_id);
-  layout->AddView(table_->CreateParentIfNecessary(), 1, 1,
-                  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
-  // been created.
-  table_->Select(0);
+  CertificateSelector::Init(text);
 }
 
-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;
+net::X509Certificate* SSLClientCertificateSelector::GetSelectedCertForTesting()
+    const {
+  return CertificateSelector::GetSelectedCert();
 }
 
-///////////////////////////////////////////////////////////////////////////////
-// SSLClientAuthObserver implementation:
-
 void SSLClientCertificateSelector::OnCertSelectedByNotification() {
   DVLOG(1) << __FUNCTION__;
   GetWidget()->Close();
 }
 
-///////////////////////////////////////////////////////////////////////////////
-// DialogDelegateView implementation:
-
-bool SSLClientCertificateSelector::CanResize() const {
-  return true;
-}
-
-base::string16 SSLClientCertificateSelector::GetWindowTitle() const {
-  return l10n_util::GetStringUTF16(IDS_CLIENT_CERT_DIALOG_TITLE);
-}
-
-void SSLClientCertificateSelector::DeleteDelegate() {
+bool SSLClientCertificateSelector::Accept() {
   DVLOG(1) << __FUNCTION__;
-  delete this;
-}
-
-bool SSLClientCertificateSelector::IsDialogButtonEnabled(
-    ui::DialogButton button) const {
-  if (button == ui::DIALOG_BUTTON_OK)
-    return !!GetSelectedCert();
-  return true;
+  CertificateSelector::Accept();

[davidben, 2015/02/18 20:32:01]

This interaction with CertificateSelector::Accept() returning true is very
confusing. If we need to be able to change that, perhaps the callback should
return true/false.

Although, I'm kind of dubious of this logic to begin with. In the non-USE_NSS
case, we're re-entrantly calling Close() inside a DialogDelegate's Accept hook.
What if we select a second certificate or close the tab while
chrome::UnlockCertSlotIfNecessary is doing its thing? (I could believe the
dialog is such that this is impossible, but this is still quite fragile.)

Perhaps the dialog should close immediately and then we
chrome::UnlockCertSlotIfNecessary asynchronously before reporting back to the
callback.

Or if we really want the dialog to say open, maybe CertificateSelector tracks a
boolean for whether it's signaled anything already. If yes, even if the Accept
callback returns true, we don't signal anything else. Which means that boolean
means something slightly different on DialogDelegate and CertificateSelector. On
DialogDelegate, false means, "don't close the dialog; also I haven't accepted
this Accept(), so we can try hitting OK again". On CertificateSelector, it means
"I have accepted this Accept() but want to keep the dialog open for some reason;
don't signal me again. I'll call Close on my own later".

The first option seems simpler, though it is a slight behavior change on the UI.

(Oh how I hate non-interface inheritance. It gets so confusing... :-) )

[pneubeck (no reviews), 2015/02/19 15:12:24]

I removed the implementation of Accept()/Cancel() in CertificateSelector,
leaving that as a detail to sub-classes.
CertificateSelector's interface (also to sub-classes) should be clearer now.
I don't think this was a problem particular to non-interface inheritance but to
the overriding of non-abstract functions.

Should be clearer now.

+  // Unlocked() will close the dialog if a certificate was selected.
+  return false;
 }
 
-bool SSLClientCertificateSelector::Cancel() {
+void SSLClientCertificateSelector::OnCertificateSelected(
+    const scoped_refptr<net::X509Certificate>& selected_cert) {
   DVLOG(1) << __FUNCTION__;
+  // 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();
-  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 (selected_cert) {
 #if defined(USE_NSS)
     chrome::UnlockCertSlotIfNecessary(
-        cert.get(),
-        chrome::kCryptoModulePasswordClientAuth,
-        cert_request_info()->host_and_port,
-        GetWidget()->GetNativeView(),
+        selected_cert.get(), chrome::kCryptoModulePasswordClientAuth,
+        cert_request_info()->host_and_port, GetWidget()->GetNativeView(),
         base::Bind(&SSLClientCertificateSelector::Unlocked,
-                   base::Unretained(this),
-                   cert));
+                   base::Unretained(this), selected_cert));
 #else
-    Unlocked(cert.get());
+    Unlocked(selected_cert.get());
 #endif
-    return false;  // Unlocked() will close the dialog.
-  }
-
-  return false;
-}
-
-views::View* SSLClientCertificateSelector::GetInitiallyFocusedView() {
-  return table_;
-}
-
-views::View* SSLClientCertificateSelector::CreateExtraView() {
-  DCHECK(!view_cert_button_);
-  view_cert_button_ = new views::LabelButton(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 {
-  return ui::MODAL_TYPE_CHILD;
-}
-
-///////////////////////////////////////////////////////////////////////////////
-// views::ButtonListener implementation:
-
-void SSLClientCertificateSelector::ButtonPressed(
-    views::Button* sender, const ui::Event& event) {
-  if (sender == view_cert_button_) {
-    net::X509Certificate* cert = GetSelectedCert();
-    if (cert)
-      ShowCertificateViewer(web_contents_,
-                            web_contents_->GetTopLevelNativeWindow(),
-                            cert);
+  } else {
+    CertificateSelected(nullptr);
   }
 }
 
-///////////////////////////////////////////////////////////////////////////////
-// views::TableViewObserver implementation:
-void SSLClientCertificateSelector::OnSelectionChanged() {
-  GetDialogClientView()->ok_button()->SetEnabled(!!GetSelectedCert());
-}
-
-void SSLClientCertificateSelector::OnDoubleClick() {
-  if (Accept())
-    GetWidget()->Close();
-}
-
-///////////////////////////////////////////////////////////////////////////////
-// SSLClientCertificateSelector private methods:
-
-void SSLClientCertificateSelector::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);