Chromium Code Reviews
Help | Chromium Project | Sign in
(361)

Issue 2823038: Refactor SSLClientAuthHandler and certificate selection (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 9 months ago by David Benjamin
Modified:
2 years, 10 months ago
Reviewers:
Mark Mentovai, wtc, brettw, agl
CC:
chromium-reviews_chromium.org, John Grabowski, darin-cc_chromium.org, brettw-cc_chromium.org, pam+watch_chromium.org, ben+cc_chromium.org
Visibility:
Public.

Description

Refactor SSLClientAuthHandler and certificate selection

This cleans up much of the code involved in displaying a certificate selection
dialog to the user.

- Adds a new inner class to RenderViewHostDelegate (later to be populated
with more SSL things).
- Adds a helper class for TabContents' implementation.
- Moves the certificate dialogs themselves to have a common entry point.
- Makes SSLClientAuthHandler call the RVHDelegate to query the user, with the
TabContents implementation displaying the dialogs.
- Picks the correct parent window for the dialog on all platforms, instead of
relying on BrowserList::GetLastActive
- Makes the OS X implementation use an asynchronous sheet, now that we know the
parent.
- Fixes an index-mismatch problem in the OS X implementation, should we fail to create an identity.

R=agl,brettw,mark
BUG=148
TEST=selecting client certificates still works

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53231

Patch Set 1 #

Total comments: 4

Patch Set 2 : Add more to ShowSSLClientCertificateSelector's comment #

Patch Set 3 : Rebase and remove some now unnecessary includes #

Patch Set 4 : Fix linux_view; needed to explicitly include a file. #

Total comments: 6

Patch Set 5 : Address brettw's comments #

Total comments: 43

Patch Set 6 : Address mark's comments #

Total comments: 3

Patch Set 7 : Make the retain/release counts correct when a sheet is open. #

Total comments: 9

Patch Set 8 : Fix some variable naming. #

Patch Set 9 : Rebase the patch #

Unified diffs Side-by-side diffs Delta from patch set Stats (+392 lines, -110 lines) Lint Patch
A chrome/browser/cocoa/ssl_client_certificate_selector.mm View 1 2 3 4 5 6 7 1 chunk +145 lines, -0 lines 0 comments ? errors Download
A + chrome/browser/gtk/ssl_client_certificate_selector.cc View 1 2 3 4 5 4 chunks +14 lines, -5 lines 0 comments ? errors Download
M chrome/browser/renderer_host/render_view_host_delegate.h View 1 2 3 4 5 6 7 8 4 chunks +14 lines, -0 lines 0 comments ? errors Download
M chrome/browser/renderer_host/render_view_host_delegate.cc View 1 chunk +5 lines, -0 lines 0 comments ? errors Download
M chrome/browser/renderer_host/render_view_host_notification_task.h View 1 2 3 4 2 chunks +54 lines, -0 lines 0 comments ? errors Download
M chrome/browser/ssl/ssl_client_auth_handler.h View 3 chunks +8 lines, -6 lines 0 comments ? errors Download
M chrome/browser/ssl/ssl_client_auth_handler.cc View 1 2 3 4 4 chunks +29 lines, -21 lines 0 comments ? errors Download
D chrome/browser/ssl/ssl_client_auth_handler_mac.mm View 1 chunk +0 lines, -63 lines 0 comments ? errors Download
A chrome/browser/ssl_client_certificate_selector.h View 1 2 3 4 5 1 chunk +30 lines, -0 lines 0 comments ? errors Download
M chrome/browser/tab_contents/tab_contents.h View 1 2 3 4 5 6 7 8 4 chunks +8 lines, -0 lines 0 comments ? errors Download
M chrome/browser/tab_contents/tab_contents.cc View 1 2 3 4 5 6 7 8 3 chunks +11 lines, -0 lines 0 comments ? errors Download
A chrome/browser/tab_contents/tab_contents_ssl_helper.h View 1 2 3 4 1 chunk +28 lines, -0 lines 0 comments ? errors Download
A chrome/browser/tab_contents/tab_contents_ssl_helper.cc View 1 2 3 4 5 1 chunk +24 lines, -0 lines 0 comments ? errors Download
A + chrome/browser/views/ssl_client_certificate_selector_win.cc View 1 2 3 4 5 4 chunks +15 lines, -12 lines 0 comments ? errors Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 7 chunks +7 lines, -3 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 19
David Benjamin
Big patch. Unfortunately, this isn't too easy to split up. (Well, I can make the ...
3 years, 9 months ago #1
brettw
http://codereview.chromium.org/2823038/diff/1/10 File chrome/browser/ssl_client_certificate_selector.h (right): http://codereview.chromium.org/2823038/diff/1/10#newcode18 chrome/browser/ssl_client_certificate_selector.h:18: // the dialog will report back to |delegate|. Can ...
3 years, 9 months ago #2
David Benjamin
http://codereview.chromium.org/2823038/diff/1/10 File chrome/browser/ssl_client_certificate_selector.h (right): http://codereview.chromium.org/2823038/diff/1/10#newcode18 chrome/browser/ssl_client_certificate_selector.h:18: // the dialog will report back to |delegate|. On ...
3 years, 9 months ago #3
agl
I can't judge the .mm files, but otherwise LGTM.
3 years, 9 months ago #4
brettw
LGTM for everything but the .mm file. Sorry it took me so long. You should ...
3 years, 9 months ago #5
David Benjamin
+mark for the Mac stuff. mark: Could you take a look at the .mm file? ...
3 years, 9 months ago #6
Mark Mentovai
I’ve only read the files I’ve commented on. http://codereview.chromium.org/2823038/diff/23001/24001 File chrome/browser/cocoa/ssl_client_certificate_selector.mm (right): http://codereview.chromium.org/2823038/diff/23001/24001#newcode1 chrome/browser/cocoa/ssl_client_certificate_selector.mm:1: // ...
3 years, 9 months ago #7
David Benjamin
http://codereview.chromium.org/2823038/diff/23001/24001 File chrome/browser/cocoa/ssl_client_certificate_selector.mm (right): http://codereview.chromium.org/2823038/diff/23001/24001#newcode1 chrome/browser/cocoa/ssl_client_certificate_selector.mm:1: // Copyright (c) 2010 The Chromium Authors. All rights ...
3 years, 9 months ago #8
Mark Mentovai
Mac LGTM http://codereview.chromium.org/2823038/diff/23001/24001 File chrome/browser/cocoa/ssl_client_certificate_selector.mm (right): http://codereview.chromium.org/2823038/diff/23001/24001#newcode9 chrome/browser/cocoa/ssl_client_certificate_selector.mm:9: #include "app/l10n_util_mac.h" David Benjamin wrote: > On ...
3 years, 9 months ago #9
David Benjamin
(Changes were minor, so I haven't updated the CL yet. Would be good to wait ...
3 years, 9 months ago #10
Mark Mentovai
davidben@chromium.org wrote: > SSLClientCertificateSelectorCocoa object is the delegate for the sheet, > so it'll need ...
3 years, 9 months ago #11
David Benjamin
On 2010/07/13 16:20:31, Mark Mentovai wrote: > mailto:davidben@chromium.org wrote: > > SSLClientCertificateSelectorCocoa object is the ...
3 years, 9 months ago #12
Mark Mentovai
Let’s not autorelease, then!
3 years, 9 months ago #13
David Benjamin
On 2010/07/15 16:29:40, Mark Mentovai wrote: > Let’s not autorelease, then! > :-) So is ...
3 years, 9 months ago #14
Mark Mentovai
davidben@chromium.org wrote: > :-) So is there a more standard convention, or should I just ...
3 years, 9 months ago #15
David Benjamin
On 2010/07/15 17:36:54, Mark Mentovai wrote: > mailto:davidben@chromium.org wrote: > > :-) So is there ...
3 years, 9 months ago #16
Mark Mentovai
Yes, this looks good, just a couple tiny nits. http://codereview.chromium.org/2823038/diff/43001/44001 File chrome/browser/cocoa/ssl_client_certificate_selector.mm (right): http://codereview.chromium.org/2823038/diff/43001/44001#newcode28 chrome/browser/cocoa/ssl_client_certificate_selector.mm:28: ...
3 years, 9 months ago #17
David Benjamin
Renamed variables. Also rebased the patch. (In two separate steps so the diffs between things ...
3 years, 9 months ago #18
Mark Mentovai
3 years, 9 months ago #19
Mac LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6