Refactor SSLClientAuthHandler and certificate selection

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

[davidben, 2010-07-02 23:32:58]

Big patch. Unfortunately, this isn't too easy to split up. (Well, I can make the
OS X sheet thing separate if folks would prefer, but not much more than that.)

Also, since I have ~no Obj-C experience at all, the Mac code may need extra
reviewing. I mostly adapted existing code though.

[brettw, 2010-07-07 18:51:52]

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 you add a little more here about the use of delegate? In particular, that
the delegate should be notified in all cases, including cancel (I think you
require this or you get confused, right?).

http://codereview.chromium.org/2823038/diff/1/13
File chrome/browser/tab_contents/tab_contents_ssl_helper.cc (right):

http://codereview.chromium.org/2823038/diff/1/13#newcode21
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:21:
ShowSSLClientCertificateSelector(tab_contents_->GetMessageBoxRootWindow(),
Is this all the code you anticipate having in this SSL class? I know I told you
to try to keep things out of TabContents, but if it's only one statement, I
think it would make TabContents simpler overall if this was just in there
(rather than having a getter for this separate interface).

If you will be adding a bunch more stuff here (more methods, etc.) then this
approach is best.

[davidben, 2010-07-07 20:30:23]

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 2010/07/07 18:51:53, brettw wrote:
> Can you add a little more here about the use of delegate? In particular, that
> the delegate should be notified in all cases, including cancel (I think you
> require this or you get confused, right?).

Done. (Yup, although it might not be necessary, since the delegate's destructor
now cleans up. Shrug.)

http://codereview.chromium.org/2823038/diff/1/13
File chrome/browser/tab_contents/tab_contents_ssl_helper.cc (right):

http://codereview.chromium.org/2823038/diff/1/13#newcode21
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:21:
ShowSSLClientCertificateSelector(tab_contents_->GetMessageBoxRootWindow(),
I do intend to add more. See [1]. Sorry that wasn't clear. (I should probably
add you to that CL...)

[1] http://codereview.chromium.org/2815023/show

[agl, 2010-07-08 21:12:31]

I can't judge the .mm files, but otherwise LGTM.

[brettw, 2010-07-12 04:31:22]

LGTM for everything but the .mm file. Sorry it took me so long. You should find
a mac weenie to check the mm file for you. I can help if you need a pointer to
finding somebody.

http://codereview.chromium.org/2823038/diff/17001/18005
File chrome/browser/renderer_host/render_view_host_notification_task.h (right):

http://codereview.chromium.org/2823038/diff/17001/18005#newcode342
chrome/browser/renderer_host/render_view_host_notification_task.h:342: inline
void CallRenderViewHostSSLDelegate(int render_process_id,
I presume you're going to use all the variants you wrote here? I probably
wouldn't bother writing ones for a number of args you don't need, given the
length of the code.

http://codereview.chromium.org/2823038/diff/17001/18006
File chrome/browser/ssl/ssl_client_auth_handler.cc (right):

http://codereview.chromium.org/2823038/diff/17001/18006#newcode32
chrome/browser/ssl/ssl_client_auth_handler.cc:32:
scoped_refptr<SSLClientAuthHandler> self(this);
Is this required or can you just write scoped_refptr<...>  as the last arg for
CallRenderViewHostSSLDelegate below? There's enough room there that you can put
it inline without looking too ugly. When I saw this, I thought you were trying
to make sure the object doesn't get deleted out from under you, which we do from
time to time.

http://codereview.chromium.org/2823038/diff/17001/18013
File chrome/browser/tab_contents/tab_contents_ssl_helper.h (right):

http://codereview.chromium.org/2823038/diff/17001/18013#newcode23
chrome/browser/tab_contents/tab_contents_ssl_helper.h:23: TabContents
*tab_contents_;
Normally we put the * on the type and not the var.

[davidben, 2010-07-12 18:39:38]

+mark for the Mac stuff.

mark: Could you take a look at the .mm file? I don't really know Obj-C. (Made
that file by taking pieces of the old
chrome/browser/ssl/ssl_client_auth_handler_mac.mm and
chrome/browser/ssl/ssl_add_cert_handler_mac.mm.)

http://codereview.chromium.org/2823038/diff/17001/18005
File chrome/browser/renderer_host/render_view_host_notification_task.h (right):

http://codereview.chromium.org/2823038/diff/17001/18005#newcode342
chrome/browser/renderer_host/render_view_host_notification_task.h:342: inline
void CallRenderViewHostSSLDelegate(int render_process_id,
On 2010/07/12 04:31:24, brettw wrote:
> I presume you're going to use all the variants you wrote here? I probably
> wouldn't bother writing ones for a number of args you don't need, given the
> length of the code.

I could probably get rid of the no-argument one. Seemed 0, 1, and 2 are fairly
standard from the others. But I guess they're probably all used.

http://codereview.chromium.org/2823038/diff/17001/18006
File chrome/browser/ssl/ssl_client_auth_handler.cc (right):

http://codereview.chromium.org/2823038/diff/17001/18006#newcode32
chrome/browser/ssl/ssl_client_auth_handler.cc:32:
scoped_refptr<SSLClientAuthHandler> self(this);
On 2010/07/12 04:31:24, brettw wrote:
> Is this required or can you just write scoped_refptr<...>  as the last arg for
> CallRenderViewHostSSLDelegate below? There's enough room there that you can
put
> it inline without looking too ugly. When I saw this, I thought you were trying
> to make sure the object doesn't get deleted out from under you, which we do
from
> time to time.

Done.

http://codereview.chromium.org/2823038/diff/17001/18013
File chrome/browser/tab_contents/tab_contents_ssl_helper.h (right):

http://codereview.chromium.org/2823038/diff/17001/18013#newcode23
chrome/browser/tab_contents/tab_contents_ssl_helper.h:23: TabContents
*tab_contents_;
On 2010/07/12 04:31:24, brettw wrote:
> Normally we put the * on the type and not the var.

Done. (Oops. C habits.)

[Mark Mentovai, 2010-07-12 19:32:40]

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: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
Be sure to set svn:eol-style = LF (sometimes this doesn’t show up on codereview
even when it would be committed with the correct property, like in certain svn
configurations, or for git users).

http://codereview.chromium.org/2823038/diff/23001/24001#newcode9
chrome/browser/cocoa/ssl_client_certificate_selector.mm:9: #include
"app/l10n_util_mac.h"
The general rule is to #import Objective-C[++] headers, and #include C[++]
headers. This one is an #import. So is scoped_nsobject.h.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode19
chrome/browser/cocoa/ssl_client_certificate_selector.mm:19: @interface
SSLClientCertificateSelectorCocoa : NSObject
The { goes on this line - the same way we do it with classes in C++.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode22
chrome/browser/cocoa/ssl_client_certificate_selector.mm:22:
scoped_refptr<SSLClientAuthHandler> handler_;
I havent’t seen an #include for ref_counted.h in this file or in its header.
That means that if this compiles, it’s relying on some other header #including
ref_counted.h. That’s inappropriate - what if that other header (or those
headers) change their implementations and no longer use ref_counted.h? This
would break. You should include it explicitly.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode30
chrome/browser/cocoa/ssl_client_certificate_selector.mm:30: -
(void)displayDialog:(gfx::NativeWindow)parent;
You should comment the behavior here. Specifically, the “releases self” bit is
interesting, so you should point it out here. This is a better place for the
note than at the very end of the ShowSSLClientCertificateSelector function
implementation.

Another option would be to not use the “releases self” behavior, but have
ShowSSLClientCertificateSelector autorelease the object it allocates before
calling -displayDialog:. This would be more in-line with how Cocoa code usually
works.

http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Prefe...

(since the SSLClientCertificateSelectorCocoa object never needs to live beyond
the scope of ShowSSLClientCertificateSelector, right?)

Apple has some docs on autoreleasing if you’re unfamiliar with Cocoa memory
management, a search for |cocoa autorelease| should find them.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode47
chrome/browser/cocoa/ssl_client_certificate_selector.mm:47: DCHECK(handler);
Need logging.h?

http://codereview.chromium.org/2823038/diff/23001/24001#newcode50
chrome/browser/cocoa/ssl_client_certificate_selector.mm:50: if (self) {
We generally write:

if ((self = [super init]))

http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Example

This is of course functionally identical, it’s just what people are used to
reading and writing in Google Objective-C[++].

http://codereview.chromium.org/2823038/diff/23001/24001#newcode63
chrome/browser/cocoa/ssl_client_certificate_selector.mm:63: NSUInteger index =
[identities_ indexOfObject:(id)[panel_ identity]];
Did you really need the (id) cast here?

http://codereview.chromium.org/2823038/diff/23001/24001#newcode64
chrome/browser/cocoa/ssl_client_certificate_selector.mm:64: DCHECK(index !=
NSNotFound);
I’m not sure about the exact operation of SFChooseIdentityPanel in this regard.
Does it constrain the user to choosing exactly one certificate? (I think it
does.)

If it’s possible in the UI to choose zero certificates, this should be handled
with something more than a DCHECK: at the very least, this should be an |if| to
protect the next line.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode78
chrome/browser/cocoa/ssl_client_certificate_selector.mm:78:
identities_.reset([NSMutableArray arrayWithCapacity:num_certs]);
-[NSMutableArray arrayWithCapacity:] gives you an autoreleased NSMutableArray*,
but you’re treating it as owned and sticking it in a member. This will
ultimately cause the object to be asked to release itself after it’s been
deallocated: a use-after-free bug.

You want [[NSMutableArray alloc] initWithCapacity:num_certs].

In Cocoa, generally, if you’re dealing with class Example:

[Example example]

will give you an autoreleased instance, equivalent to:

[[[Example alloc] init] autorelease]

Since the initial ownership reference is balanced by an autorelease, you don’t
need to do anything else to release it (and it’s an error to do so). For
something that you want to keep your own ownership reference on, such as
something that lives in a class’ member variable (and possibly has its ownership
managed by scoped_nsobject<>), you need to keep that initial ownership claim,
like:

[[Example alloc] init]

http://codereview.chromium.org/2823038/diff/23001/24001#newcode83
chrome/browser/cocoa/ssl_client_certificate_selector.mm:83: if
(SecIdentityCreateWithCertificate(NULL, cert, &identity) == noErr) {
If this function fails, the indices in the identities_ NSArray will not match
the indicies in the cert_request_info_->client_certs vector.
-sheetDidEnd:returnCode:context: assumes that the indices are compatible.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode97
chrome/browser/cocoa/ssl_client_certificate_selector.mm:97: NSString* domain =
base::SysUTF8ToNSString(
Fantastic, this isn’t in Apple’s documentation, but it shows up in the public
header. :(

Does this show up in the UI anywhere?

The header says:

If the user chooses an identity and a domain has been set, an identity
preference item will be created in the default keychain. Subsequently, calling
SecIdentitySearchCreateWithPolicy and SecIdentitySearchCopyNext will return the
preferred identity for this domain first.

which I guess makes it important to get the value for this “right.” Why did you
choose the format with https:// and the host and port as you did? Are you
following any example code? Is it interoperable with what another browser (or
browsers) do by observation? Are you sure https will always be the right
protocol?

http://codereview.chromium.org/2823038/diff/23001/24001#newcode100
chrome/browser/cocoa/ssl_client_certificate_selector.mm:100: [panel_
setInformativeText:message];
You should set the default button title too, because we have more localizations
available to us than the system does, and it’s possible that we’ve translated OK
to a language that the system doesn’t know about.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode101
chrome/browser/cocoa/ssl_client_certificate_selector.mm:101: [panel_
setAlternateButtonTitle:l10n_util::GetNSString(IDS_CANCEL)];
Does command-. or Esc work to trigger the cancel button in this dialog?

http://codereview.chromium.org/2823038/diff/23001/24001#newcode113
chrome/browser/cocoa/ssl_client_certificate_selector.mm:113: contextInfo:NULL
Use nil to represent “no Objective-C object,” not NULL.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode119
chrome/browser/cocoa/ssl_client_certificate_selector.mm:119: [self
sheetDidEnd:panel_ returnCode:returnCode context:NULL];
NULL -> nil, again.

(Other uses of NULL in this file that I haven’t commented on are correct.)

http://codereview.chromium.org/2823038/diff/23001/24009
File chrome/browser/ssl_client_certificate_selector.h (right):

http://codereview.chromium.org/2823038/diff/23001/24009#newcode1
chrome/browser/ssl_client_certificate_selector.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
New file? 2010.

http://codereview.chromium.org/2823038/diff/23001/24009#newcode21
chrome/browser/ssl_client_certificate_selector.h:21: void
ShowSSLClientCertificateSelector(
Namespace me?

[davidben, 2010-07-12 22:03:07]

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 reserved.
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> Be sure to set svn:eol-style = LF (sometimes this doesn’t show up on
codereview
> even when it would be committed with the correct property, like in certain svn
> configurations, or for git users).

I am in fact using git. I did put the stuff into my .subversion/config, so I
believe it'll be fine.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode9
chrome/browser/cocoa/ssl_client_certificate_selector.mm:9: #include
"app/l10n_util_mac.h"
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> The general rule is to #import Objective-C[++] headers, and #include C[++]
> headers. This one is an #import. So is scoped_nsobject.h.

Hrm. The existing code seems inconsistent, but the #includes far outnumber the
#imports. Does the rule apply for C++ code which wraps or convert to Obj-C
objects? l10n_util_mac.h declares only symbols in a namespace, and
scoped_nsobject.h defines a couple of templates.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode19
chrome/browser/cocoa/ssl_client_certificate_selector.mm:19: @interface
SSLClientCertificateSelectorCocoa : NSObject
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> The { goes on this line - the same way we do it with classes in C++.

Done.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode22
chrome/browser/cocoa/ssl_client_certificate_selector.mm:22:
scoped_refptr<SSLClientAuthHandler> handler_;
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> I havent’t seen an #include for ref_counted.h in this file or in its header.
> That means that if this compiles, it’s relying on some other header #including
> ref_counted.h. That’s inappropriate - what if that other header (or those
> headers) change their implementations and no longer use ref_counted.h? This
> would break. You should include it explicitly.

Done.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode30
chrome/browser/cocoa/ssl_client_certificate_selector.mm:30: -
(void)displayDialog:(gfx::NativeWindow)parent;
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> You should comment the behavior here. Specifically, the “releases self” bit is
> interesting, so you should point it out here. This is a better place for the
> note than at the very end of the ShowSSLClientCertificateSelector function
> implementation.
> 
> Another option would be to not use the “releases self” behavior, but have
> ShowSSLClientCertificateSelector autorelease the object it allocates before
> calling -displayDialog:. This would be more in-line with how Cocoa code
usually
> works.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Prefe...
> 
> (since the SSLClientCertificateSelectorCocoa object never needs to live beyond
> the scope of ShowSSLClientCertificateSelector, right?)
> 
> Apple has some docs on autoreleasing if you’re unfamiliar with Cocoa memory
> management, a search for |cocoa autorelease| should find them.

I've made it autorelease. (Does beginSheetForWindow take a reference on the
modalDelegate parameter? If not, the object will need to live a little bit
longer than that function.)

http://codereview.chromium.org/2823038/diff/23001/24001#newcode47
chrome/browser/cocoa/ssl_client_certificate_selector.mm:47: DCHECK(handler);
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> Need logging.h?

Done.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode50
chrome/browser/cocoa/ssl_client_certificate_selector.mm:50: if (self) {
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> We generally write:
> 
> if ((self = [super init]))
> 
> http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Example
> 
> This is of course functionally identical, it’s just what people are used to
> reading and writing in Google Objective-C[++].

Done.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode63
chrome/browser/cocoa/ssl_client_certificate_selector.mm:63: NSUInteger index =
[identities_ indexOfObject:(id)[panel_ identity]];
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> Did you really need the (id) cast here?

It was in the original code. :-) I just tried to remove it and got a compiler
error, so it seems some cast is necessary.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode64
chrome/browser/cocoa/ssl_client_certificate_selector.mm:64: DCHECK(index !=
NSNotFound);
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> I’m not sure about the exact operation of SFChooseIdentityPanel in this
regard.
> Does it constrain the user to choosing exactly one certificate? (I think it
> does.)
> 
> If it’s possible in the UI to choose zero certificates, this should be handled
> with something more than a DCHECK: at the very least, this should be an |if|
to
> protect the next line.

It shouldn't be possible to hit OK and not select exactly one certificate.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode78
chrome/browser/cocoa/ssl_client_certificate_selector.mm:78:
identities_.reset([NSMutableArray arrayWithCapacity:num_certs]);
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> -[NSMutableArray arrayWithCapacity:] gives you an autoreleased
NSMutableArray*,
> but you’re treating it as owned and sticking it in a member. This will
> ultimately cause the object to be asked to release itself after it’s been
> deallocated: a use-after-free bug.
> 
> You want [[NSMutableArray alloc] initWithCapacity:num_certs].
> 
> In Cocoa, generally, if you’re dealing with class Example:
> 
> [Example example]
> 
> will give you an autoreleased instance, equivalent to:
> 
> [[[Example alloc] init] autorelease]
> 
> Since the initial ownership reference is balanced by an autorelease, you don’t
> need to do anything else to release it (and it’s an error to do so). For
> something that you want to keep your own ownership reference on, such as
> something that lives in a class’ member variable (and possibly has its
ownership
> managed by scoped_nsobject<>), you need to keep that initial ownership claim,
> like:
> 
> [[Example alloc] init]

Makes sense. Done.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode83
chrome/browser/cocoa/ssl_client_certificate_selector.mm:83: if
(SecIdentityCreateWithCertificate(NULL, cert, &identity) == noErr) {
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> If this function fails, the indices in the identities_ NSArray will not match
> the indicies in the cert_request_info_->client_certs vector.
> -sheetDidEnd:returnCode:context: assumes that the indices are compatible.

Eww. This problem was in the original code, so the CL doesn't really make it
worse, but I've added a fix.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode97
chrome/browser/cocoa/ssl_client_certificate_selector.mm:97: NSString* domain =
base::SysUTF8ToNSString(
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> Fantastic, this isn’t in Apple’s documentation, but it shows up in the public
> header. :(
> 
> Does this show up in the UI anywhere?
> 
> The header says:
> 
> If the user chooses an identity and a domain has been set, an identity
> preference item will be created in the default keychain. Subsequently, calling
> SecIdentitySearchCreateWithPolicy and SecIdentitySearchCopyNext will return
the
> preferred identity for this domain first.
> 
> which I guess makes it important to get the value for this “right.” Why did
you
> choose the format with https:// and the host and port as you did? Are you
> following any example code? Is it interoperable with what another browser (or
> browsers) do by observation? Are you sure https will always be the right
> protocol?

This was in the original code. See [1]. It was written by snej, but I seem to
recall Wan-teh telling me he was no longer around? The other place I see
reference to this is in X509Certificate::GetSSLClientCertificates which also
prepends the https. I can only assume that it is there for interoperability with
Safari and that Safari does the same thing. https should be the correct protocol
given that these are SSL certificates, but that is rather ugly (though this CL
does not make it any worse).

It does not appear to show up in the actual UI.

I've added a comment about the preference business, since I wasn't even aware of
that behavior. :-) Not actually sure I like it either...

[1] http://codereview.chromium.org/2823038/diff/23001/24008

http://codereview.chromium.org/2823038/diff/23001/24001#newcode100
chrome/browser/cocoa/ssl_client_certificate_selector.mm:100: [panel_
setInformativeText:message];
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> You should set the default button title too, because we have more
localizations
> available to us than the system does, and it’s possible that we’ve translated
OK
> to a language that the system doesn’t know about.

Done.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode101
chrome/browser/cocoa/ssl_client_certificate_selector.mm:101: [panel_
setAlternateButtonTitle:l10n_util::GetNSString(IDS_CANCEL)];
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> Does command-. or Esc work to trigger the cancel button in this dialog?

Seems to. Is that correct, or should I do something about it?

http://codereview.chromium.org/2823038/diff/23001/24001#newcode113
chrome/browser/cocoa/ssl_client_certificate_selector.mm:113: contextInfo:NULL
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> Use nil to represent “no Objective-C object,” not NULL.

Done.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode119
chrome/browser/cocoa/ssl_client_certificate_selector.mm:119: [self
sheetDidEnd:panel_ returnCode:returnCode context:NULL];
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> NULL -> nil, again.
> 
> (Other uses of NULL in this file that I haven’t commented on are correct.)

Done.

http://codereview.chromium.org/2823038/diff/23001/24009
File chrome/browser/ssl_client_certificate_selector.h (right):

http://codereview.chromium.org/2823038/diff/23001/24009#newcode1
chrome/browser/ssl_client_certificate_selector.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> New file? 2010.

Oh, oops. Done.

http://codereview.chromium.org/2823038/diff/23001/24009#newcode21
chrome/browser/ssl_client_certificate_selector.h:21: void
ShowSSLClientCertificateSelector(
On 2010/07/12 19:32:43, Mark Mentovai wrote:
> Namespace me?

Namespaced under browser. (Is that the correct namespace for this? I wasn't able
to discern too much of a convention, but it seemed most of the dialogs live
there.)

[Mark Mentovai, 2010-07-12 22:27:41]

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 2010/07/12 19:32:43, Mark Mentovai wrote:
> > The general rule is to #import Objective-C[++] headers, and #include C[++]
> > headers. This one is an #import. So is scoped_nsobject.h.
> 
> Hrm. The existing code seems inconsistent, but the #includes far outnumber the
> #imports. Does the rule apply for C++ code which wraps or convert to Obj-C
> objects? l10n_util_mac.h declares only symbols in a namespace, and
> scoped_nsobject.h defines a couple of templates.

This might help clarify:

1. Never #import from non-Objective files.

2. Never #import (or #include) an Objective file from non-Objective. It won’t
work.

3. Exception: some headers, such as l10n_util_mac.h, are specifically written to
be #included or #imported from non-Objective files. This generally requires the
use of the __OBJC__ macro to “hide” Objective-C from C++, or to provide a
C++-compatible fallback. l10n_util_mac.h has an example of this, where an
Objective-C or C++-style forward declaration for NSString is chosen based on
whether Objective is available.

For headers that fall into this exception, treat them as Objective when working
in an Objective file such as this one, and use #import.

#import for l10n_util_mac.h and scoped_nsobject.h. The rest remain #include.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode30
chrome/browser/cocoa/ssl_client_certificate_selector.mm:30: -
(void)displayDialog:(gfx::NativeWindow)parent;
David Benjamin wrote:
> I've made it autorelease. (Does beginSheetForWindow take a reference on the
> modalDelegate parameter?

I doubt it.

> If not, the object will need to live a little bit
> longer than that function.)

It will. It’s guaranteed to live longer than ShowSSLClientCertificateSelector.
The actual release will come when the event loop cycles. (This code had better
be running on the UI thread or something else equipped with an autorelease
pool—I didn’t mention this before because it involves a UI dialog, so it ought
to be on an appropriate thread for so many other reasons.)

http://codereview.chromium.org/2823038/diff/23001/24001#newcode97
chrome/browser/cocoa/ssl_client_certificate_selector.mm:97: NSString* domain =
base::SysUTF8ToNSString(
Yeah, I don’t know if this was ever really thought out or defended. OK to leave
it as it was before, but it’d be nice to resolve the question for real. It
deserved a comment before (not blaming you for that).

snej is no longer here, we can’t ask him.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode101
chrome/browser/cocoa/ssl_client_certificate_selector.mm:101: [panel_
setAlternateButtonTitle:l10n_util::GetNSString(IDS_CANCEL)];
David Benjamin wrote:
> Seems to. Is that correct, or should I do something about it?

Yup. Perfect.

http://codereview.chromium.org/2823038/diff/32001/33001#newcode49
chrome/browser/cocoa/ssl_client_certificate_selector.mm:49: self = [super init];
Weird indentation through here. A local variable wouldn’t kill you.

http://codereview.chromium.org/2823038/diff/32001/33001#newcode96
chrome/browser/cocoa/ssl_client_certificate_selector.mm:96:
panel_.reset([[SFChooseIdentityPanel alloc] init]);
That’s better!

[davidben, 2010-07-12 23:01:19]

(Changes were minor, so I haven't updated the CL yet. Would be good to wait
until mstone-6 is branched to push this anyway.)

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"
On 2010/07/12 22:27:43, Mark Mentovai wrote:
> [...]
> #import for l10n_util_mac.h and scoped_nsobject.h. The rest remain #include.

Makes sense. Done.

http://codereview.chromium.org/2823038/diff/23001/24001#newcode30
chrome/browser/cocoa/ssl_client_certificate_selector.mm:30: -
(void)displayDialog:(gfx::NativeWindow)parent;
On 2010/07/12 22:27:43, Mark Mentovai wrote:
> It will. It’s guaranteed to live longer than ShowSSLClientCertificateSelector.
> The actual release will come when the event loop cycles. (This code had better
> be running on the UI thread or something else equipped with an autorelease
> pool—I didn’t mention this before because it involves a UI dialog, so it ought
> to be on an appropriate thread for so many other reasons.)

Right, this code is happening on the UI thread. Could probably use a DCHECK
while I'm at it. It's apparently already including chrome_thread.h anyway...

SSLClientCertificateSelectorCocoa object is the delegate for the sheet, so it'll
need to last until the the dialog is closed, not just longer than the function.
Or does the event loop actually not cycle before that happens? The sheet seems
asynchronous...

http://codereview.chromium.org/2823038/diff/23001/24001#newcode97
chrome/browser/cocoa/ssl_client_certificate_selector.mm:97: NSString* domain =
base::SysUTF8ToNSString(
On 2010/07/12 22:27:43, Mark Mentovai wrote:
> Yeah, I don’t know if this was ever really thought out or defended. OK to
leave
> it as it was before, but it’d be nice to resolve the question for real. It
> deserved a comment before (not blaming you for that).
> 
> snej is no longer here, we can’t ask him.

Yeah. I'll revisit it when I get to poking at how we remember certificates,
since it's pretty wonky right now anyway. For now, leaving it as-is is probably
a safe thing to do.

http://codereview.chromium.org/2823038/diff/32001/33001#newcode49
chrome/browser/cocoa/ssl_client_certificate_selector.mm:49: self = [super init];
On 2010/07/12 22:27:43, Mark Mentovai wrote:
> Weird indentation through here. A local variable wouldn’t kill you.

Hehe. Done.

[Mark Mentovai, 2010-07-13 16:20:31]

davidben@chromium.org wrote:
> SSLClientCertificateSelectorCocoa object is the delegate for the sheet,
> so it'll need to last until the the dialog is closed, not just longer
> than the function. Or does the event loop actually not cycle before that
> happens? The sheet seems asynchronous...

The docs seemed to imply that it was synchronous. I thought it said
something like “the sheet will be killed by the time this returns.”
Could you verify that?

[davidben, 2010-07-14 00:15:59]

On 2010/07/13 16:20:31, Mark Mentovai wrote:
> mailto:davidben@chromium.org wrote:
> > SSLClientCertificateSelectorCocoa object is the delegate for the sheet,
> > so it'll need to last until the the dialog is closed, not just longer
> > than the function. Or does the event loop actually not cycle before that
> > happens? The sheet seems asynchronous...
> 
> The docs seemed to imply that it was synchronous. I thought it said
> something like “the sheet will be killed by the time this returns.”
> Could you verify that?
> 

beginSheetForWindow returns before the sheet is dismissed, so it doesn't look
like autorelease should work.

[Mark Mentovai, 2010-07-15 16:29:40]

Let’s not autorelease, then!

[davidben, 2010-07-15 17:03:08]

On 2010/07/15 16:29:40, Mark Mentovai wrote:
> Let’s not autorelease, then!
> 

:-) So is there a more standard convention, or should I just go back to the
"release when sheet closes" thing? I could do a [self retain] just before
opening the sheet and a [self release] in the callback, so the caller at least
needn't care. Or is that not kosher?

[Mark Mentovai, 2010-07-15 17:36:54]

davidben@chromium.org wrote:
> :-) So is there a more standard convention, or should I just go back to the
> "release when sheet closes" thing? I could do a [self retain] just before
> opening the sheet and a [self release] in the callback, so the caller at
> least needn't care. Or is that not kosher?

It’s probably best to do your own retain, and then release in the
callback. That way, you’re covered regardless of how your “owner”
decides to manage you.

Mark

[davidben, 2010-07-15 18:25:39]

On 2010/07/15 17:36:54, Mark Mentovai wrote:
> mailto:davidben@chromium.org wrote:
> > :-) So is there a more standard convention, or should I just go back to the
> > "release when sheet closes" thing? I could do a [self retain] just before
> > opening the sheet and a [self release] in the callback, so the caller at
> > least needn't care. Or is that not kosher?
> 
> It’s probably best to do your own retain, and then release in the
> callback. That way, you’re covered regardless of how your “owner”
> decides to manage you.
> 
> Mark
> 

Yeah, that's what I meant with the [self release]. This version look good to
you?

[Mark Mentovai, 2010-07-15 18:32:54]

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:
scoped_refptr<net::SSLCertRequestInfo> cert_request_info_;
certRequestInfo_ (per the other change).

http://codereview.chromium.org/2823038/diff/43001/44001#newcode38
chrome/browser/cocoa/ssl_client_certificate_selector.mm:38:
certRequestInfo:(net::SSLCertRequestInfo*)cert_request_info;
This too.

http://codereview.chromium.org/2823038/diff/43001/44001#newcode46
chrome/browser/cocoa/ssl_client_certificate_selector.mm:46:
net::SSLCertRequestInfo* cert_request_info,
This one is correct as-is because it’s outside of an @interface/@implementation.

http://codereview.chromium.org/2823038/diff/43001/44001#newcode61
chrome/browser/cocoa/ssl_client_certificate_selector.mm:61:
certRequestInfo:(net::SSLCertRequestInfo*)cert_request_info {
(Fix throughout.)

http://codereview.chromium.org/2823038/diff/43001/44001#newcode127
chrome/browser/cocoa/ssl_client_certificate_selector.mm:127: // runs.
sheetDidEnd:returnCode:context: will release the additional
Ought to be -sheetDidEnd:returnCode:context: with a leading hyphen to identify
it as an instance (as opposed to class, with a plus) method.

[davidben, 2010-07-15 19:11:25]

Renamed variables. Also rebased the patch. (In two separate steps so the diffs
between things should be readable.)

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:
scoped_refptr<net::SSLCertRequestInfo> cert_request_info_;
On 2010/07/15 18:32:55, Mark Mentovai wrote:
> certRequestInfo_ (per the other change).

Done.

http://codereview.chromium.org/2823038/diff/43001/44001#newcode38
chrome/browser/cocoa/ssl_client_certificate_selector.mm:38:
certRequestInfo:(net::SSLCertRequestInfo*)cert_request_info;
On 2010/07/15 18:32:55, Mark Mentovai wrote:
> This too.

Done.

http://codereview.chromium.org/2823038/diff/43001/44001#newcode61
chrome/browser/cocoa/ssl_client_certificate_selector.mm:61:
certRequestInfo:(net::SSLCertRequestInfo*)cert_request_info {
On 2010/07/15 18:32:55, Mark Mentovai wrote:
> (Fix throughout.)

Done.

http://codereview.chromium.org/2823038/diff/43001/44001#newcode127
chrome/browser/cocoa/ssl_client_certificate_selector.mm:127: // runs.
sheetDidEnd:returnCode:context: will release the additional
On 2010/07/15 18:32:55, Mark Mentovai wrote:
> Ought to be -sheetDidEnd:returnCode:context: with a leading hyphen to identify
> it as an instance (as opposed to class, with a plus) method.

Done.

[Mark Mentovai, 2010-07-15 19:12:51]

Mac LGTM