Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
932553002:
Refactor SSLClientCertificateSelector for reuse with platformKeys API. (Closed)
|
Created:
5 years, 10 months ago by pneubeck (no reviews) Modified:
5 years, 10 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@cert_perms Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor SSLClientCertificateSelector for reuse with platformKeys API.
platformKeys API also must open a dialog for client certificate selection.
Compared to SSLClientCertificateSelector, the API's dialog
- does not need synchronization of multiple open dialogs on different tabs of the same origin.
- does not need slot unlocking, as it is implemented on ChromeOS only.
While there fixing the tests of the selector: The test certificates were imported without private key and thus the Unlock crashed because the NSS slot was null.
BUG=121007, 103534, 103529, 450167
Committed: https://crrev.com/556a7a8fa9ae4f716a418c998e4b3add079abb45
Cr-Commit-Position: refs/heads/master@{#317319}
Patch Set 1 : #Patch Set 2 : Adding namespace 'chrome'. #Patch Set 3 : Cleaned up includes. Fixed compilation. #
Total comments: 15
Patch Set 4 : Addressed comments, some cleanup like const-ness, CHECKs, logging, includes, ... #
Total comments: 69
Patch Set 5 : Addressed Bartosz's comments #Patch Set 6 : Rebased. #Patch Set 7 : Addressed Mike's comment. #
Total comments: 2
Patch Set 8 : Removed Init overwriting. #
Total comments: 14
Patch Set 9 : Fixed comments. #Messages
Total messages: 39 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
pneubeck@chromium.org changed reviewers: + davidben@chromium.org
@Benjamin, please take a look.
Patchset #1 (id:80001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #1 (id:100001) has been deleted
On 2015/02/16 10:28:17, pneubeck wrote: > @Benjamin, please take a look. Hah, had Benjamin Kalman to often as reviewer. I meant David, of course :-)
pneubeck@chromium.org changed reviewers: + sky@chromium.org
Also adding an owner. @Scott, ptal.
https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:25: namespace chrome { To confirm: is //chrome namespaced these days? I remember it used to not be, but it seems to be a bit of a wash right now. https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:22: typedef std::vector<scoped_refptr<X509Certificate>> CertificateList; :-( It looks like everything else duplicates the typedef though, so okay. https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:39: const scoped_refptr<net::X509Certificate>& selected_cert)>; Note: this'll conflict with this CL. I expect yours will land first, so I can rebase mine. Probably we'd want to swap out the callback with that delegate interface or something similar. Would that work for platformKeys? https://codereview.chromium.org/859213006/ https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:65: CertificateSelector::Accept(); 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... :-) ) https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.h:27: public SSLClientAuthObserver { Neither of these base classes is much of a pure interface, and it took me a bit to follow what was going on there. Ownership of dialogs is odd and we do override Accept on it, so perhaps embed SSLClientAuthObserver and give it a callback for OnCertSelectedByNotification? This was already true before though, so this is maybe should be left for a follow-up. https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.h:41: // DialogDelegateView: chrome::CertificatorSelector: https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc (left): https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc:140: cert_request_info_1_->client_certs.push_back(mit_davidben_cert_); Aww, my old internship test cert is going away. :-)
Patchset #4 (id:190001) has been deleted
Patchset #4 (id:190001) has been deleted
ptal https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:25: namespace chrome { On 2015/02/18 20:32:01, David Benjamin wrote: > To confirm: is //chrome namespaced these days? I remember it used to not be, but > it seems to be a bit of a wash right now. I found a few places that use 'namespace chrome'. Hard to say whether that are outliers or whether it's the current trend. I followed my preference to not crowd the global namespace. Just for documentation: https://code.google.com/p/chromium/codesearch#search/&q=namespace%5C%20chrome... (searching for "namespace\ chrome\ {" in header files) reports 172 matches. https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:22: typedef std::vector<scoped_refptr<X509Certificate>> CertificateList; On 2015/02/18 20:32:01, David Benjamin wrote: > :-( > > It looks like everything else duplicates the typedef though, so okay. Acknowledged. https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:39: const scoped_refptr<net::X509Certificate>& selected_cert)>; On 2015/02/18 20:32:01, David Benjamin wrote: > Note: this'll conflict with this CL. I expect yours will land first, so I can > rebase mine. Probably we'd want to swap out the callback with that delegate > interface or something similar. Would that work for platformKeys? > > https://codereview.chromium.org/859213006/ Thinking more about it, I have the impression that for platformKeys it doesn't really matter. For HTTPS, IIUC, the reasoning is that we persist the choice of 'selecting no certificate'. (We should actually make that explicit in the dialog instead of hiding that in a Cancel button) platformKeys itself doesn't persist the selection. It's the decision of the extension using the API whether and what it persists. So this differentiation doesn't really have any meaning for the API. https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:65: CertificateSelector::Accept(); On 2015/02/18 20:32:01, David Benjamin wrote: > 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. > 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. > (Oh how I hate non-interface inheritance. It gets so confusing... :-) ) 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. https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.h:27: public SSLClientAuthObserver { On 2015/02/18 20:32:02, David Benjamin wrote: > Neither of these base classes is much of a pure interface, and it took me a bit > to follow what was going on there. Ownership of dialogs is odd and we do > override Accept on it, so perhaps embed SSLClientAuthObserver and give it a > callback for OnCertSelectedByNotification? > > This was already true before though, so this is maybe should be left for a > follow-up. Acknowledged. https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.h:41: // DialogDelegateView: On 2015/02/18 20:32:01, David Benjamin wrote: > chrome::CertificatorSelector: Done. https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc (left): https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc:140: cert_request_info_1_->client_certs.push_back(mit_davidben_cert_); On 2015/02/18 20:32:02, David Benjamin wrote: > Aww, my old internship test cert is going away. :-) that explains it finally :-)
pneubeck@chromium.org changed reviewers: + msw@chromium.org - sky@chromium.org
-Scott, +Mike Mike, ptal. David already reviewed it but I also need an owner lgtm.
bartfab@chromium.org changed reviewers: + bartfab@chromium.org
A couple of drive-by nits. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:44: for (const scoped_refptr<net::X509Certificate>& cert : certificates) { Why not |auto|? https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:85: views::GridLayout* const layout = views::GridLayout::CreatePanel(this); Nit: Use a |scoped_ptr| while you are holding ownership. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:88: const int column_set_id = 0; 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. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:90: column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1, If you have a single column only, why not use a BoxLayout? https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:94: views::Label* const label = new views::Label(text); Nit: Use a |scoped_ptr| while you are holding ownership. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:137: DVLOG(1) << __FUNCTION__; Do we expect crashes around this line? https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:154: view_cert_button_ = new views::LabelButton( Nit: Use a |scoped_ptr| while you are holding ownership. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:186: views::TableView* const table = new views::TableView( Nit: Use a |scoped_ptr| while you are holding ownership. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:36: // Sub-classes can get the current certificate using |GetSelectedCert()|. The Why can only subclasses get the certificate? GetSelectedCert() is a public method. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:37: // explnatory text shown to the user must be provided to |Init()|. Nit: s/explnatory/explanatory/ https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:51: // the user what the implication of the certificate selection is. Nit: s/the/to the/ https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:74: views::TableView* CreateCertTable(); Nit: Return a scoped_ptr.
https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:44: for (const scoped_refptr<net::X509Certificate>& cert : certificates) { On 2015/02/19 17:53:06, bartfab wrote: > Why not |auto|? I find it helpful to document the element type, in particular if the container type is already typedef'ed away. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:85: views::GridLayout* const layout = views::GridLayout::CreatePanel(this); On 2015/02/19 17:53:07, bartfab wrote: > Nit: Use a |scoped_ptr| while you are holding ownership. 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()); ... https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:88: const int column_set_id = 0; On 2015/02/19 17:53:06, bartfab wrote: > Nit 1: Use are using inconsistent naming for constant,s |column_set_id| vs. > |kTableViewWidth|. Done. > Nit 2: Define constants at the top of the file. Not sure what you intend to improve by that. This go against the typical attempt to define stuff where you use it. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:90: column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1, On 2015/02/19 17:53:06, bartfab wrote: > If you have a single column only, why not use a BoxLayout? Agreed. I can try that in a follow up. This CL tries not to change the existing SSLClientCertificateSelector dialog. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:94: views::Label* const label = new views::Label(text); On 2015/02/19 17:53:06, bartfab wrote: > Nit: Use a |scoped_ptr| while you are holding ownership. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:137: DVLOG(1) << __FUNCTION__; On 2015/02/19 17:53:06, bartfab wrote: > Do we expect crashes around this line? I don't know, kept this from the SSLClientCertificateSelector. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:154: view_cert_button_ = new views::LabelButton( On 2015/02/19 17:53:06, bartfab wrote: > Nit: Use a |scoped_ptr| while you are holding ownership. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:186: views::TableView* const table = new views::TableView( On 2015/02/19 17:53:06, bartfab wrote: > Nit: Use a |scoped_ptr| while you are holding ownership. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:36: // Sub-classes can get the current certificate using |GetSelectedCert()|. The On 2015/02/19 17:53:07, bartfab wrote: > Why can only subclasses get the certificate? GetSelectedCert() is a public > method. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:37: // explnatory text shown to the user must be provided to |Init()|. On 2015/02/19 17:53:07, bartfab wrote: > Nit: s/explnatory/explanatory/ Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:51: // the user what the implication of the certificate selection is. On 2015/02/19 17:53:07, bartfab wrote: > Nit: s/the/to the/ Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:74: views::TableView* CreateCertTable(); On 2015/02/19 17:53:07, bartfab wrote: > Nit: Return a scoped_ptr. Done.
https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:45: const base::string16 text = l10n_util::GetStringFUTF16( nit: inline this in the push_back call. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:123: if (selected >= 0 && static_cast<size_t>(selected) < certificates_.size()) We should be able to [D]CHECK_LT(selected, certificates_.size()), right? I guess maybe it would fail if called before Init()... https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:137: DVLOG(1) << __FUNCTION__; On 2015/02/19 17:53:06, bartfab wrote: > Do we expect crashes around this line? If not, this override isn't needed. DialogDelegateView's impl would suffice. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:142: if (button == ui::DIALOG_BUTTON_OK) nit: return button != ui::DIALOG_BUTTON_OK || GetSelectedCert() != nullptr; https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:186: views::TableView* const table = new views::TableView( On 2015/02/19 17:53:06, bartfab wrote: > Nit: Use a |scoped_ptr| while you are holding ownership. Why not assign directly to table_, as this code used to do, or inline in Init? https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:24: typedef std::vector<scoped_refptr<X509Certificate>> CertificateList; nit: avoid the X509Certificate forward decl and duplicate CertificateList typedef, by including net/cert/x509_certificate.h here, rather than in the cc. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:81: views::TableView* table_ = nullptr; Do not assign default values here, use the constructor's initializer list. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector_browsertest.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector_browsertest.cc:26: ASSERT_TRUE(client_1_); nit: ASSERT_NE to nullptr; ditto below. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector_browsertest.cc:50: // The selector will be deleted when the browser is shutdown. Perhaps explicitly close the |selector_| dialog in TearDownOnMainThread? https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector_browsertest.cc:51: chrome::CertificateSelector* selector_ = nullptr; Use the initializer list; don't assign here. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:17: #include "ui/views/controls/button/label_button.h" nit: Remove this; audit other includes. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:18: #include "ui/views/controls/label.h" nit: Remove this; audit other includes. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:19: #include "ui/views/controls/table/table_view.h" nit: Remove this; audit other includes. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:20: #include "ui/views/layout/grid_layout.h" nit: Remove this; audit other includes. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:21: #include "ui/views/layout/layout_constants.h" nit: Remove this; audit other includes. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:23: #include "ui/views/window/dialog_client_view.h" nit: Remove this; audit other includes. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:26: #include "chrome/browser/ui/crypto_module_password_dialog_nss.h" nit: is this needed? is chrome/browser/ui/crypto_module_password_dialog.h ok? https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.h:35: void Init(); nit: avoid overloading the non-virtual base class method name. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.h:45: void CreateCertTable(); nit: avoid overloading the non-virtual base class method name. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc:62: ASSERT_NE(static_cast<net::X509Certificate*>(NULL), client_cert_1_.get()); nit: nullptr (makes static_cast unnecessary?); ditto below.
Patchset #7 (id:270001) has been deleted
Thanks Mike! https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:45: const base::string16 text = l10n_util::GetStringFUTF16( On 2015/02/19 19:53:53, msw wrote: > nit: inline this in the push_back call. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:123: if (selected >= 0 && static_cast<size_t>(selected) < certificates_.size()) On 2015/02/19 19:53:53, msw wrote: > We should be able to [D]CHECK_LT(selected, certificates_.size()), right? > I guess maybe it would fail if called before Init()... A call before Init() will crash because of table_ == nullptr. I added a comment to the declaration. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:137: DVLOG(1) << __FUNCTION__; On 2015/02/19 19:53:53, msw wrote: > On 2015/02/19 17:53:06, bartfab wrote: > > Do we expect crashes around this line? > > If not, this override isn't needed. DialogDelegateView's impl would suffice. good point. removed. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:142: if (button == ui::DIALOG_BUTTON_OK) On 2015/02/19 19:53:53, msw wrote: > nit: return button != ui::DIALOG_BUTTON_OK || GetSelectedCert() != nullptr; Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:186: views::TableView* const table = new views::TableView( On 2015/02/19 19:53:53, msw wrote: > On 2015/02/19 17:53:06, bartfab wrote: > > Nit: Use a |scoped_ptr| while you are holding ownership. > > Why not assign directly to table_, as this code used to do, or inline in Init? The old one was weird with respect to ownership and it was not documented/obvious that it modifies some state. inlining seems fine. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:24: typedef std::vector<scoped_refptr<X509Certificate>> CertificateList; On 2015/02/19 19:53:53, msw wrote: > nit: avoid the X509Certificate forward decl and duplicate CertificateList > typedef, by including net/cert/x509_certificate.h here, rather than in the cc. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:81: views::TableView* table_ = nullptr; On 2015/02/19 19:53:53, msw wrote: > Do not assign default values here, use the constructor's initializer list. I wonder why, considering that this doesn't require to synchronize with the initializer list and should thus be less error-prone. It's also style-guide conformant. anyways, keeping it consistent with views/ code, I guess. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector_browsertest.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector_browsertest.cc:26: ASSERT_TRUE(client_1_); On 2015/02/19 19:53:53, msw wrote: > nit: ASSERT_NE to nullptr; ditto below. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector_browsertest.cc:50: // The selector will be deleted when the browser is shutdown. On 2015/02/19 19:53:53, msw wrote: > Perhaps explicitly close the |selector_| dialog in TearDownOnMainThread? Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector_browsertest.cc:51: chrome::CertificateSelector* selector_ = nullptr; On 2015/02/19 19:53:53, msw wrote: > Use the initializer list; don't assign here. As I understand the style-guide this is the preferred way as there's no need for an explicitly defined constructor in this case. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Initialization "Decision: Use in-class member initialization for simple initializations [...]" https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:17: #include "ui/views/controls/button/label_button.h" On 2015/02/19 19:53:53, msw wrote: > nit: Remove this; audit other includes. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:18: #include "ui/views/controls/label.h" On 2015/02/19 19:53:53, msw wrote: > nit: Remove this; audit other includes. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:19: #include "ui/views/controls/table/table_view.h" On 2015/02/19 19:53:53, msw wrote: > nit: Remove this; audit other includes. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:20: #include "ui/views/layout/grid_layout.h" On 2015/02/19 19:53:53, msw wrote: > nit: Remove this; audit other includes. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:21: #include "ui/views/layout/layout_constants.h" On 2015/02/19 19:53:53, msw wrote: > nit: Remove this; audit other includes. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:23: #include "ui/views/window/dialog_client_view.h" On 2015/02/19 19:53:53, msw wrote: > nit: Remove this; audit other includes. Done. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:26: #include "chrome/browser/ui/crypto_module_password_dialog_nss.h" On 2015/02/19 19:53:53, msw wrote: > nit: is this needed? is chrome/browser/ui/crypto_module_password_dialog.h ok? chrome::UnlockCertSlotIfNecessary is defined there and also not forwarded through crypto_module_password_dialog.h https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.h:35: void Init(); On 2015/02/19 19:53:53, msw wrote: > nit: avoid overloading the non-virtual base class method name. I can come up with any other random name (e.g. Initialize()), but Init() seems to be the most typical? Note that this also hides the base class declaration and cannot be called without explicit name qualification. I'm fine with either. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.h:45: void CreateCertTable(); On 2015/02/19 19:53:54, msw wrote: > nit: avoid overloading the non-virtual base class method name. deleted. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc:62: ASSERT_NE(static_cast<net::X509Certificate*>(NULL), client_cert_1_.get()); On 2015/02/19 19:53:54, msw wrote: > nit: nullptr (makes static_cast unnecessary?); ditto below. Done.
Thanks for reworking the Accept bit. That's much clearer. I've filed crbug/460215 for the double-Accept thing since that's not new with this CL. lgtm https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:39: const scoped_refptr<net::X509Certificate>& selected_cert)>; On 2015/02/19 15:12:24, pneubeck wrote: > On 2015/02/18 20:32:01, David Benjamin wrote: > > Note: this'll conflict with this CL. I expect yours will land first, so I can > > rebase mine. Probably we'd want to swap out the callback with that delegate > > interface or something similar. Would that work for platformKeys? > > > > https://codereview.chromium.org/859213006/ > > Thinking more about it, I have the impression that for platformKeys it doesn't > really matter. > For HTTPS, IIUC, the reasoning is that we persist the choice of 'selecting no > certificate'. (We should actually make that explicit in the dialog instead of > hiding that in a Cancel button) (Agreed. That UI is terrible. Though we'll still need the separate plumbing outside of there.) > platformKeys itself doesn't persist the selection. It's the decision of the > extension using the API whether and what it persists. So this differentiation > doesn't really have any meaning for the API. Alright. Well, I'll route them into the same one if it ends up applying. But it looks this new separation between the base class makes it no longer matter. https://codereview.chromium.org/932553002/diff/290001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc (right): https://codereview.chromium.org/932553002/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc:52: #if defined(USE_NSS) Nit: maybe a comment for why we import the key here, but not in the non-NSS case.
lgtm with minor renaming comment. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector_browsertest.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector_browsertest.cc:51: chrome::CertificateSelector* selector_ = nullptr; On 2015/02/19 20:43:49, pneubeck wrote: > On 2015/02/19 19:53:53, msw wrote: > > Use the initializer list; don't assign here. > > As I understand the style-guide this is the preferred way as there's no need for > an explicitly defined constructor in this case. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Initialization > "Decision: Use in-class member initialization for simple initializations [...]" Interesting; I suppose the style has changed since I last looked. Odd that this guide seems different from the internal guide link that I have: https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.cc:26: #include "chrome/browser/ui/crypto_module_password_dialog_nss.h" On 2015/02/19 20:43:49, pneubeck wrote: > On 2015/02/19 19:53:53, msw wrote: > > nit: is this needed? is chrome/browser/ui/crypto_module_password_dialog.h ok? > > chrome::UnlockCertSlotIfNecessary is defined there and also not forwarded > through crypto_module_password_dialog.h Acknowledged. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.h:35: void Init(); On 2015/02/19 20:43:50, pneubeck wrote: > On 2015/02/19 19:53:53, msw wrote: > > nit: avoid overloading the non-virtual base class method name. > > I can come up with any other random name (e.g. Initialize()), but Init() seems > to be the most typical? > Note that this also hides the base class declaration and cannot be called > without explicit name qualification. > > I'm fine with either. Consider CertificateSelector::InitAndShow, since it also triggers the dialog to actually be shown (which is rather odd for an Init function). It'd be nice if Init was just rolled into the ctor (with a new text arg) and chrome::ShowSSLClientCertificateSelector called constrained_window::ShowWebModalDialogViews explicitly, but then I guess something else would need to select the first row of the table to start, if that actually needs to be done after the dialog is created/shown.
pneubeck@chromium.org changed reviewers: + jcivelli@chromium.org
pneubeck@chromium.org changed required reviewers: + davidben@chromium.org, jcivelli@chromium.org, msw@chromium.org
jcivelli@chromium.org: Please review changes in chrome/test/BUILD.gn
LGTM for chrome/test/BUILD.gn
New patchsets have been uploaded after l-g-t-m from davidben@chromium.org,msw@chromium.org,jcivelli@chromium.org
@Bartosz, ptal at the last changes (patchset 7 to 8) https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector.h:35: void Init(); On 2015/02/19 21:03:31, msw wrote: > On 2015/02/19 20:43:50, pneubeck wrote: > > On 2015/02/19 19:53:53, msw wrote: > > > nit: avoid overloading the non-virtual base class method name. > > > > I can come up with any other random name (e.g. Initialize()), but Init() seems > > to be the most typical? > > Note that this also hides the base class declaration and cannot be called > > without explicit name qualification. > > > > I'm fine with either. > > Consider CertificateSelector::InitAndShow, since it also triggers the dialog to > actually be shown (which is rather odd for an Init function). > > It'd be nice if Init was just rolled into the ctor (with a new text arg) and > chrome::ShowSSLClientCertificateSelector called > constrained_window::ShowWebModalDialogViews explicitly, but then I guess > something else would need to select the first row of the table to start, if that > actually needs to be done after the dialog is created/shown. I considered the splitting as well. It makes the Init() behave more as expected but that doesn't help with the overriding. Regarding putting the initialization into the c'tor, I think that is more harmful than good in C++, also the style guide explicitly disallows that: "Avoid doing complex initialization in constructors (in particular, initialization that can fail or that requires virtual method calls)." I renamed the Init function in the base class to InitWithText and made it protected. https://codereview.chromium.org/932553002/diff/290001/chrome/browser/ui/views... File chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc (right): https://codereview.chromium.org/932553002/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc:52: #if defined(USE_NSS) On 2015/02/19 20:54:20, David Benjamin wrote: > Nit: maybe a comment for why we import the key here, but not in the non-NSS > case. Done.
I had another look at the certificate_selector.* files I had nitted on before. These LGTM. https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/210001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:85: views::GridLayout* const layout = views::GridLayout::CreatePanel(this); On 2015/02/19 19:43:51, pneubeck wrote: > On 2015/02/19 17:53:07, bartfab wrote: > > Nit: Use a |scoped_ptr| while you are holding ownership. > > 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()); > ... 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. https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:60: DCHECK_LT(static_cast<size_t>(index), items_.size()); Nit: #include "base/basictypes.h" https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:129: if (selected < 0) Just curious: Is this TableView's way of saying "nothing selected"? https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:8: #include <vector> This: This is only needed in the implementation file now. https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:11: #include "base/memory/ref_counted.h" This: This is only needed in the implementation file now. https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:30: // A base class for dialogs that shows a given list of certificates to the user. Nit: s/shows/show/ https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:47: // Must be called after Init(). Nit: This documentation needs updating. There is no Init() anymore. https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:51: // certificate. Nit: Does InitWithText() need to be called before Show()? If so, you may want to document that.
New patchsets have been uploaded after l-g-t-m from bartfab@chromium.org
https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:60: DCHECK_LT(static_cast<size_t>(index), items_.size()); On 2015/02/20 12:19:47, bartfab wrote: > Nit: #include "base/basictypes.h" Done. https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:129: if (selected < 0) On 2015/02/20 12:19:47, bartfab wrote: > Just curious: Is this TableView's way of saying "nothing selected"? yes exactly. added a comment. https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.h (right): https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:8: #include <vector> On 2015/02/20 12:19:47, bartfab wrote: > This: This is only needed in the implementation file now. Done. https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:11: #include "base/memory/ref_counted.h" On 2015/02/20 12:19:47, bartfab wrote: > This: This is only needed in the implementation file now. Done. https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:30: // A base class for dialogs that shows a given list of certificates to the user. On 2015/02/20 12:19:47, bartfab wrote: > Nit: s/shows/show/ Done. https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:47: // Must be called after Init(). On 2015/02/20 12:19:47, bartfab wrote: > Nit: This documentation needs updating. There is no Init() anymore. Done. https://codereview.chromium.org/932553002/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.h:51: // certificate. On 2015/02/20 12:19:47, bartfab wrote: > Nit: Does InitWithText() need to be called before Show()? If so, you may want to > document that. Done.
The CQ bit was checked by pneubeck@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/932553002/330001
Message was sent while issue was closed.
Committed patchset #9 (id:330001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/556a7a8fa9ae4f716a418c998e4b3add079abb45 Cr-Commit-Position: refs/heads/master@{#317319} |
